-
Notifications
You must be signed in to change notification settings - Fork 231
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
Wrong usage of Sobol' sequence #414
Comments
Hi @tupui Thanks for raising this issue. I will submit a fix for this soon. I would appreciate a code review when I do so, if you are willing. Thanks also for the tip on the new scipy module. |
Of course, just ping me when you are ready :) Also, FYI I am trying to get some SA in SciPy (I have some interest as I did my thesis in SA, Saltelli was the president of my jury 😄. So I have a lot of code too: things like Sobol', moment independent, CUSUNORO, COSI). You can find the discussion here: https://mail.python.org/pipermail/scipy-dev/2021-April/024696.html There is even a suggestion to include SALib into SciPy if you would have some interest. It would be great if you or other maintainers could drop in and comment on the whole topic, this could help the discussion! |
Thanks again @tupui I've just submitted a PR for your perusal (I've tagged you in it, and you can see the link above). On the scipy discussion, I am a little short on time at the moment - I and other maintainers will take a closer look when able. |
Thanks @ConnectedSystems, I just commented on the PR. Seems to be good. It makes sense not to raise for now to give users time to update their code. Looking forward to read some inputs from the team on SciPy's mailing list then 😃 |
Outlined in recent discussion in SALib#414 and in scipy/scipy#10844 (comment)
I noticed that Sobol' sequence could be used in a wrong way by
sample.saltelli
:SALib/src/SALib/sample/saltelli.py
Line 43 in 6765749
The convergence properties of Sobol' sequence are only valid if sampling exactly 2^n points. Also, if skipping is wanted, you must skip 2^m points and then you can only sample 2^n points with n < m.
We had lengthly discussions while adding a QMC module to SciPy notably with Art Owen, Sergei Kucherenko and Fred Hickernell:
scipy/scipy#10844
Art Owen wrote an article about it too:
https://arxiv.org/abs/2008.08051
In the end, I would suggest to either fix the usage of Sobol' here or rely on the new
scipy.stats.qmc
which will be available in the next release of SciPy (1.7).In general, this module if fairly new (and we are always looking for people interested in contributing) but already has Sobol' sequence, Halton, LHS and discrepancy measures. Implementations of Sobol' and discrepancy are in Cython and orders of magnitudes faster than equivalent in python.
Let me know if I can help you with something.
ps. Sobol' writes with the apostrophe, it's not a mark of possessive but the diacritic is in the name.
The text was updated successfully, but these errors were encountered: