-
-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Randomized qmc #57
Randomized qmc #57
Conversation
This looks amazing--thank you so much for all the hard work!
I think it's nice having separate functions for randomization as well (in case users want to randomize themselves), but do you think you could include code that directly samples a randomized sequence? Generating randomized sequences is sometimes a lot faster--see this paper, for example, which gets speedups of several orders of magnitude. You don't have to add code for generating these in-place, though; we can get that done in a future PR. For now, we can just include some kind of randomized constructor like:
And have this generate the deterministic point set, before randomizing with the methods you've added. We can work on higher-performance implementations later.
Mostly this is because of what users will need when using a point set. Users typically iterate over an array one point at a time, rather than one dimension at a time, so we want it to be in an efficient format for that. Memory layout is probably not going to be the bottleneck for any realistic application, though.
Hmm, possibly, but I'm not 100% sure. There's easy gains are from using either |
It's column major for performance assuming you using slices/views of points. |
Yes, this is very important because remember that the vast majority of this kind of feature is transitive, not direct. The vast majority of users are not going to be We need to rebase this so it gets the update that makes it run the downstream tests, though currently as noted in #58 the downstream tests are broken as a prior change seems to have broke Surrogates.jl. It's a similar piece to above: the interfaces need to be solid because the way the package is used is mostly going to be transitively through other packages, so we really need to be sure that there is a single entry point that can be guaranteed and controlled simply through passing type information. |
Thanks for the feedback.
I'll change the current code to include |
Ok, I'll add a field
with default as
and then do as @ParadaCarleton says
However, I'll also export the randomization methods directly, it should not cause issue or interference with other packages. |
I am not sure what you mean by rebase. |
Thanks! Can you change the name from One nice way to let users randomize directly might be to use function objects; |
Could you illustrate a bit more what you mean? I have a hard time understanding what difference would |
I feel like some changes mentioned in #44 should be implemented directly in this PR.
where randomizer is different for each method (maybe related to your previous comment @ParadaCarleton ?).
methods which randomize in place i.e. as they generate the sample will be done separately latter, they will have specialized
Doing all that should remove plenty of repeated code lines. |
It wouldn't make a huge difference, it's just one way you could set up an interface I think would look nice, since it wouldn't require creating a randomization function separate from each object. Defining a randomizer like this would let you use this syntax:
I just find this aesthetically pleasing, but there's not a big need for it if you think otherwise.
I think it makes sense to avoid some of these if they're not directly related, e.g. renaming |
I gave @ParadaCarleton maintainer rights for this repo, so he'll be able to merge than he think it's ready. I am agreeing with his interface ideas here and he's deeper into this field than I, so I'll let him take this PR from here. Though it's a big change so I wouldn't aim for perfectionism: let's try to get something agreeable in, and then start fixing it in small bits.
This kind of stuff should be follow up PRs. When there's a giant PR like this, everyone is a bit reluctant to make other changes since then this needs to rebase onto the new master all of the time. So let's try to find a good stopping point here, get it in, and then the smaller details can be nibbled at by everyone until we're happy. Then do the high level interface overhaul to the new names and such, and that sounds like a good v1.0. |
Git works by tracking all the changes (the history) of a file, rather than by tracking the current state. If you know all the changes that have been made to a file, you can reconstruct what it looks like now, but this representation makes it easier for multiple people to work on the same code. Currently, your changes are "based" on an old version of QMC that you pulled from the server about a week ago. While you were making these changes, we made a separate bug fix to correct a problem with our implementation of Kronecker sequences. "Rebase" means you take your current branch, then change the base (from the old version you started with, to the most recently-updated version). That way, your changes are applied on top of the newest version, instead of being applied on top of the old one. You can find a tutorial on how to do this in GitKraken (a git GUI) here, or you can use some other git GUI like Github Desktop (which should have their own tutorials). |
@ChrisRackauckas @vikram-s-narayan could you explain the use case for the |
I found two mentions of In both cases, each matrice should be i.i.d. I guess. I believe this can only be achieved cleanly with multiple independent randomization of original QMC sequence. It seems to me like the current result of If the default randomization was set to something else than BTW maybe we should also consider an on place version (or iterator) for |
Right, I see. I think all of those are good ideas; @ChrisRackauckas it looks like some of these packages need much more rigorous testing, to make sure the programs are returning correct answers. It’s a huge problem that none of this was caught when reviewing the relevant PRs. |
They are passing their convergence tests. Downstream GlobalSensitivity.jl has a bunch of regression tests against other GSA libraries (like sensitivity from R) https://github.com/SciML/GlobalSensitivity.jl/blob/master/test/sobol_method.jl, the NeuralPDE library has a ton of convergence testing against analytical solutions https://github.com/SciML/NeuralPDE.jl/blob/master/test/NNPDE_tests.jl#L55 This is why the downstream tests were added. Downstream usage and applications have much more rigorous tests of the results, though they stick to a subset of samplers that we know work.
Yes, this one. We know that this one was created pretty hastily to get the interface together but the internals need more than a little bit of work. That said, all applications right now stick to the two samplers without randomization or restarting so it works fine, though is of course not optimal. Basically, we needed the interfaces to exist so it could be started to be developed on downstream, but other than the two wrap algorithms called in full the rest was left pretty WIP in QMC. The docs (https://docs.sciml.ai/QuasiMonteCarlo/dev/) pretty much scream "this is still work in progress" though, so it's at least understood that this generalization is still in progress. BTW, we should probably write up a synopsis for a QMC GSoC. Would you be interested to help mentor someone on this?
It should have an API for that, but making it simple to get actual matrices is still very necessary since it can be difficult to use that lazy generator for example in GPU kernels, which is where this tends to be used downstream the most (NeuralPDE and GSA generally try to make use of GPUs) |
Just to clarify, I said the current implementation is “kinda wrong” in the sense it is not pure QMC, however, it might still be better than MC. Hence, all the tests mentioned by @ChrisRackauckas can still pass. About QMC GSoC, I have no previous experience, but I would be happy to mentor. About in place iterator, I do not feel competent ! Besides that, for this PR I am kind of done in terms of features I wanted to add |
We should add a downstream test to GlobalSensitivity.jl though, that's a good point because it is one of the big consumers of this package right now. However, I would be heavily surprised if GSA or physics-informed neural network tests would be able to see a major difference due to this effect: it probably improves downstream, but the more general tests won't be sensitive enough to that. So definitely one of the next things we need to do is get QMC.jl to have a much more robust test set that is showing some of the finer details of the properties. I 100% agree on that.
As we tend to always do, we can merge, open an issue, and get that added later. Open source is an ecosystem and a community, no one needs to solve the whole problem.
|
In my example
Indeed, if |
README.md
Outdated
x_digital_shift = randomize(x_faure, DigitalShift(base = b, pad = pad)) | ||
x_shift = randomize(x_faure, Shift()) | ||
x_uniform = rand(d, N) # plain i.i.d. uniform | ||
``` | ||
|
||
```julia | ||
using Plots | ||
# Setting I like for plotting | ||
default(fontfamily="Computer Modern", linewidth=1, label=nothing, grid=true, framestyle=:default) | ||
``` | ||
|
||
Plot the resulting sequences along dimensions `1` and `3`. | ||
|
||
```julia | ||
d1 = 1 | ||
d2 = 3 | ||
sequences = [x_uniform, x_faure, x_shift, x_digital_shift, x_lms, x_nus] | ||
names = ["Uniform", "Faure (unrandomized)", "Shift", "Digital Shift", "Linear Matrix Scrambling", "Nested Uniform Scrambling"] | ||
p = [plot(thickness_scaling=1.5, aspect_ratio=:equal) for i in sequences] | ||
for (i, x) in enumerate(sequences) | ||
scatter!(p[i], x[d1, :], x[d2, :], ms=0.9, c=1, grid=false) | ||
title!(names[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have an extra tab in them
README.md
Outdated
begin | ||
d1 = 1 | ||
d2 = 3 | ||
x = x_nus | ||
p = [plot(thickness_scaling=1.5, aspect_ratio=:equal) for i in 0:m] | ||
for i in 0:m | ||
j = m - i | ||
xᵢ = range(0, 1, step=1 / b^(i)) | ||
xⱼ = range(0, 1, step=1 / b^(j)) | ||
scatter!(p[i+1], x[d1, :], x[d2, :], ms=2, c=1, grid=false) | ||
xlims!(p[i+1], (0, 1.01)) | ||
ylims!(p[i+1], (0, 1.01)) | ||
yticks!(p[i+1], [0, 1]) | ||
xticks!(p[i+1], [0, 1]) | ||
hline!(p[i+1], xᵢ, c=:gray, alpha=0.2) | ||
vline!(p[i+1], xⱼ, c=:gray, alpha=0.2) | ||
end | ||
plot(p..., size=(1500, 900)) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
begin | |
d1 = 1 | |
d2 = 3 | |
x = x_nus | |
p = [plot(thickness_scaling=1.5, aspect_ratio=:equal) for i in 0:m] | |
for i in 0:m | |
j = m - i | |
xᵢ = range(0, 1, step=1 / b^(i)) | |
xⱼ = range(0, 1, step=1 / b^(j)) | |
scatter!(p[i+1], x[d1, :], x[d2, :], ms=2, c=1, grid=false) | |
xlims!(p[i+1], (0, 1.01)) | |
ylims!(p[i+1], (0, 1.01)) | |
yticks!(p[i+1], [0, 1]) | |
xticks!(p[i+1], [0, 1]) | |
hline!(p[i+1], xᵢ, c=:gray, alpha=0.2) | |
vline!(p[i+1], xⱼ, c=:gray, alpha=0.2) | |
end | |
plot(p..., size=(1500, 900)) | |
end | |
d1 = 1 | |
d2 = 3 | |
x = x_nus | |
p = [plot(thickness_scaling=1.5, aspect_ratio=:equal) for i in 0:m] | |
for i in 0:m | |
j = m - i | |
xᵢ = range(0, 1, step=1 / b^(i)) | |
xⱼ = range(0, 1, step=1 / b^(j)) | |
scatter!(p[i+1], x[d1, :], x[d2, :], ms=2, c=1, grid=false) | |
xlims!(p[i+1], (0, 1.01)) | |
ylims!(p[i+1], (0, 1.01)) | |
yticks!(p[i+1], [0, 1]) | |
xticks!(p[i+1], [0, 1]) | |
hline!(p[i+1], xᵢ, c=:gray, alpha=0.2) | |
vline!(p[i+1], xⱼ, c=:gray, alpha=0.2) | |
end | |
plot(p..., size=(1500, 900)) |
This looks good to me, though I don't know about that test function. If you put a reference to it I'll take a look there. I'll let @ParadaCarleton take a stroll through this though with a finer comb. |
The test is effectively the same as my old test for |
So then is this good to merge? Do we call this 1.0? |
Still haven’t had a chance to review this, but I will soon. I’d definitely hold off on 1.0 for now, though (there’s still quite a bit left before that). |
@dmetivie Do you think this is ready for review? |
Also, could you move the information you've added to the README to a new page in the docs? The README should really just be a brief description of the package and what it does, rather than comprehensive documentation about all the different methods. |
Yes! I put all the text I wanted for now in the doc and simplified the Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few minor fixes missing!
Project.toml
Outdated
ConcreteStructs = "2569d6c7-a4a2-43d3-a901-331e8e4be471" | ||
Distributions = "31c24e10-a181-5473-b8eb-7969acd0382f" | ||
IntervalArithmetic = "d1acc4aa-44c8-5952-acd4-ba5d80a2a253" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this addition necessary? It looks like it's only a test dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep istms
in scr
as I suggested in another comment, we need to keep that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep
istms
inscr
as I suggested in another comment, we need to keep that one
I think istms
is neat, but we don’t want to add new dependencies for it, especially heavy ones like IntervalArithmetic
. I’d prefer moving istms
to the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not too concerned with dependency in general, so of course do as you see fit for this package.
Maybe some day we could do a QuasiMonteCarloUI.jl
package with this function and other discrepancy measurements, something practitioners would not care too much about, but theoretician would like.
@@ -1,5 +1,5 @@ | |||
""" | |||
KroneckerSample(generator::AbstractVector) <: SamplingAlgorithm | |||
KroneckerSample(generator::AbstractVector) <: DeterministicSamplingAlgorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for introducing the "Deterministic Sampling Algorithm" type? I'm not sure we need different types for the two kinds of algorithms, given you can turn a deterministic algorithm into a randomized one quite easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree I should document that at some point to clarify.
Most classical QMC are deterministic (with the NoRand()
option). Of course, they can all been randomized, but at their core they are deterministic. Whereas, other random sequences like LHS and uniform are random at their core.
The distinction becomes important when using generate_design_matrices
.
If you want independent realization for a Random Algorithm: easy just sample several times (see code), no need for randomize
. For Deterministic Algorithm, the independent matrix will be made using randomize
.
If we were to implement fast scrambled Sobol like in this paper you mentioned, then we would classify it as RandomAlgo as the randomization is done while creating the sequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn’t the same algorithm applicable for both sets of matrices? In both cases we can take . I know the current implementation doesn’t randomize while creating the sequence, but creating multiple sequences with a randomizer other than NoRand
should generate independent matrices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I first introduced that to keep the default with NoRand
as it is now, i.e. generating different matrices. If we use the same method as RandomAlgo
it will produce with NoRand
identical matrix and probably makes downstream bug.
I do agree that for NoRand
it is not too crazy to expect that generating matrix is useless as it should always generate the same matrix. However, it seems that some people use [out[(j * d + 1):((j + 1) * d), :] for j in 0:(num_mats - 1)]
.
The second reasons, is that the way I coded DeterministicAlgo
- Produces the
NoRand
sequence - Then randomize it in place several times. It is faster, especially for scrambling
Whereas[sample(n, d, sampler(R= RandMethod), T) for j in 1:num_mats]
is longer.
Third, (but again this is personal taste) I find it nice to classify algo by random or deterministic nature. (Especially for people not very familiar with QMC?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense. Looks good!
So this is my first PR :).
Thanks for @ParadaCarleton and @ChrisRackauckas for encouraging that.
The purpose of this PR is to add randomization methods by merging the RandomizedQuasiMonteCarlo.jl (which is now meant to be deprecated).
In particular, there are scrambling methods as Nested Uniform Scrambling and Linear Matrix Scrambling and Digital Shift in arbitrary base
b
.These methods are used to randomize deterministic Quasi Monte Carlo methods. In general RQMC has better performance than QMC plus it allows error estimations.
Compare to other packages like SciPy or qmcpy or piece of code you can find on internet, the randomization is done independently of the sequence. In other API, you choose at generation if you want a randomize version or not (see my post).
I have multiple questions and comments attached to this PR
— As for now, arrays
x
are of the formn×dim
whilesample
produces adims×n
array.I wondered why this latter convention was decided? Is it because Distributions.jl do like that? If yes, can someone explain why this choice?
In scrambling code, it seems the opposite seem more consistent with column major computing (randomization is done independently for each dimension).
— About the API, I tend to want to separate RQMC methods from deterministic QMC methods
rand(ShiftMethod(), x)
. However, I understand one would want something like thatsample(m, SamplingMethod(rand = ShiftMethod()))
.So far, I just give the RQMC methods as functions of a point set (see the Readme.md).
nested_uniform_scramble(x, b; M = M)
linear_matrix_scramble(x, base; M = M)
.digital_shift(x, base; M = M)
.shift(x)
.where
b
is the base used to scramble andM
the number of bits in baseb
used to represent digits.— Julia question: would it be relevant (and more efficient) to create a class for “binary expansion” in base
b≥2
and related operation (likexor
).In the code, I have a lot of
AbstractArrays
of<:Integer
. In base 2 it should simplifies to<:Bool
.I am asking about some kind of type “
Bool{b}
" which allows Integers from 0 to b-1 only.When
b=2
it would be the good oldBool
with certainly performance increase.