Add Sobol' sampler from SciPy #519
Conversation
|
I tried to update the ubuntu version in the CI but it would need more work. So probably better not to touch this here. |
|
Thanks @tupui I will take a closer look when I can (may take a week or two) but at first glance this looks like a very clean re-implementation. Re skipping vs scrambling, I would prefer to support skipping anyway, as I think the user should be able to decide what to apply. One thing I would like to see is expanded docstrings with more complete notes to cover the differences (I can do this if you like). |
|
Great thanks @ConnectedSystems. It took me quite some time to do this as well 😉
It's highly based on the current Saltelli sampler. I just removed the skipping part.
But what is the use case? As seen in #518. Skipping bring difficulties if one want's to resample. And it's also a complex logic to keep the properties. Instead, if the use case is to have different samples, I would suggest adding a This brings up the
Sure, I can do this once we settle on the API. |
I understand the reservation - my view is that SALib is useful as a teaching tool as well as a research tool. Someone out there may be relying on it for some purpose (e.g., to show case different strategies). The code itself is useful if one were interested in seeing how skipping might be implemented.
I think |
|
Ok I can add back the skip parameter following what's in the current Saltelli method. And I will add seed. |
|
I did the update. |
| qrng = qmc.Sobol(d=2*D, scramble=scramble, seed=seed) | ||
|
|
||
| # fast-forward logic | ||
| if skip_values > 0: |
There was a problem hiding this comment.
Is it acceptable if skip_values > 0 and scramble == True?
There was a problem hiding this comment.
Whatever the answer, it would be worthwhile to add it to the docstring I think
There was a problem hiding this comment.
Yes we can combine both. I added a note in the docstrings as suggested.
| saltelli_sequence = np.zeros([(Dg + 2) * N, D]) | ||
| index = 0 | ||
|
|
||
| for i in range(N): |
There was a problem hiding this comment.
Not completely necessary but I suggest we replace these nested loops with itertools.product()
There was a problem hiding this comment.
I am not sure how to go about this. The loops need to update
| qrng = qmc.Sobol(d=2*D, scramble=scramble, seed=seed) | ||
|
|
||
| # fast-forward logic | ||
| if skip_values > 0: |
There was a problem hiding this comment.
Yes we can combine both. I added a note in the docstrings as suggested.
| saltelli_sequence = np.zeros([(Dg + 2) * N, D]) | ||
| index = 0 | ||
|
|
||
| for i in range(N): |
There was a problem hiding this comment.
I am not sure how to go about this. The loops need to update index in the first level. Maybe this can be kept for latter. And actually there might be another way to write these with NumPy operations.
|
@ConnectedSystems the CI failure is also present on |
|
I updated the PR with the recent changes in |
Closes #486, closes #450 and should close #478 as if the method does work better (which I am not convinced yet from my tests) should go in SciPy directly.
Following discussions in #486, this adds Sobol' sampler as a new sampler and deprecate
salib.sample.saltelli.Note that SciPy also has LHS (and Halton that can be used otherwise).
This needs SciPy to 1.7 and doing so I updated NumPy and Python minimal versions following NEP29.
This is a draft as I need some guidance on how to proceed next. E.g. I removed the skipping as we can scramble and should have better properties doing so.
cc @ConnectedSystems