Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use randomized sobol sampling #167
base: master
Are you sure you want to change the base?
Use randomized sobol sampling #167
Changes from 10 commits
52a924d
8142a18
846f306
a0d9364
04ab43b
d060c3c
8fa44eb
ff2dc3c
65f5d8d
facbac8
3b7cf99
19e83ad
73c74eb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This should be changed as well, shouldn't it?
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.
Reading the comment from the PR that added RQMC that you had put in the issue. The OP mentions
So it makes sense to enforce the randomization only for bootstrap cases, what do you think? Changing this is significantly affecting the results of these trivial tests and the indices values for this are pretty commonly accepted, though to be honest I guess the new values could be justified a bit better I am just worried about users from R trying to match the values and then having to defend this
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.
Regardless of what this PR ends up doing: I think the tests, examples and recommendations should consistently do the same thing as the
gsa
implementations withsamples
kwarg (is this function tested at all if the changes do not affect the tests?).My main concern is that generally doing non-standard things with low discrepancy sequences is a bad idea. E.g., even just omitting the initial point of zeros (as done in Sobol.jl) is a bad idea, as discussed in the issue in Sobol.jl and shown in https://arxiv.org/pdf/2008.08051. What's currently done in GlobalSensitivity - and what IMO rightfully shows a warning to users - is that the
samples * num_mats
samples are generated by taking a single consecutive subset ofsamples * num_mats
and partitioning it: https://github.com/SciML/QuasiMonteCarlo.jl/blob/5c5483565d5b6a083256861bcf7e00cf7075c5f4/src/RandomizedQuasiMonteCarlo/iterators.jl#L268-L271 In general this won't preserve any of the distribution properties of the original sequence.Switching to a proper RQMC approach would be much more sound. However, as far as I know, generally it shouldn't lead to (much) different/worse estimates of integrals, so something seems off or some other part of the algorithm seems to interact badly with the randomized sequences. A quick search reveals quite a few papers though regarding global sensitivity and RQMC, so I think it should work and be an improvement in general.
Regardless of the mathematical details, IMO just sticking with the current implementation is not a good idea for another reason: Users might get the same results as with other (R) packages - but every time they run
gsa
without pre-computed design matrices they will see a warning.So in case it's too unclear yet how to fix the RQMC issues, I suggest as a temporary workaround to just do a single
sample
call (as in the implementation ofgenerate_design_matrices
that's currently used) in tests, examples, andgsa
that at least makes it apparent how the samples are generated and fixes the warning (and should even lead to a more efficientgsa
implementation since some of the partitioning andhcat
ing can be avoided, only a single split is needed). Probably it would be good to still add a comment about this choice and links to RQMC in the docs.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.
This was pretty helpful, I need to get to it @devmotion though sorry for the delay.
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 something blocking or making your work harder?