Use randomized sobol sampling

[Vaibhavdixit02]

Checklist

• Appropriate tests were added
• Any code changes were done in a way that does not break public API
• All documentation related to code changes were updated
• The new code follows the
  contributor guidelines, in particular the SciML Style Guide and
  COLPRAC.
• Any new documentation only uses public API

Additional context

Add any other context about the problem here.

Fixes #166

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.83%. Comparing base (71fcc16) to head (facbac8).

❗ Current head facbac8 differs from pull request most recent head 73c74eb

Please upload reports for the commit 73c74eb to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage   92.78%   92.83%   +0.04%     
==========================================
  Files          11       11              
  Lines         832      837       +5     
==========================================
+ Hits          772      777       +5     
  Misses         60       60              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[devmotion]

This should be changed as well, shouldn't it?

[Vaibhavdixit02]

Reading the comment from the PR that added RQMC that you had put in the issue. The OP mentions

To nuance my words, for the use case M=2 of sensitivity analysis this is probably not as bad as I show for small enough cases.

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

[devmotion]

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 with samples 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 of samples * 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 of generate_design_matrices that's currently used) in tests, examples, and gsa that at least makes it apparent how the samples are generated and fixes the warning (and should even lead to a more efficient gsa implementation since some of the partitioning and hcating 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.

[Vaibhavdixit02]

This was pretty helpful, I need to get to it @devmotion though sorry for the delay.

[Vaibhavdixit02]

Is this something blocking or making your work harder?