Skip to content
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

We could merge the QMCSampler and MC sampler #372

Open
wedeling opened this issue May 31, 2022 · 3 comments
Open

We could merge the QMCSampler and MC sampler #372

wedeling opened this issue May 31, 2022 · 3 comments
Assignees

Comments

@wedeling
Copy link
Collaborator

The QMCSampler does not work with DiscreteUniform inputs, and I see no apparent way of fixing this, see #368. In short, the problem lies with the external subroutine from SALib, which generates the Saltelli sampling plan, and with the inverse transform of DiscreteUniform in chaospy, which generates non-integer values.

The MCSampler has a built-in Saltelli algorithm that does not have these issues.

Further, the only difference between QMC / MCSampler is that the former uses a space-filling design from SALib. But this is also available in ChaosPy: https://chaospy.readthedocs.io/en/master/user_guide/fundamentals/quasi_random_samples.html?highlight=latin. So there is no need for SALib, and we could drop this dependency. We would just need to pass a flag to the MCSampler if we want to use this, for instance rule='latin_hypercube'.

Finally, I think I've discussed this before, but the names MCSampler and QMCSampler are misleading. Both generate a Saltelli sampling plan, which is specifically designed for Sobol indices. This is why you get n_mc_samples * (d + 2) samples (d is the number of inputs), if you specify that you want n_mc_samples samples. The RandomSampler we have is a "pure" MC sampler. So we could change the name of MCSampler or we should make it clear that it's designed for sensitivity analysis.

@DavidPCoster
Copy link
Collaborator

If we have two routines offering the same functionality for no good reason, then I think it makes sense to remove one of them. And then I think it makes sense to remove the one that introduces an additional dependency.

Would it make sense to add a "deprecation" output message to the QMCSampler routine at this release and then remove it at the next release? And update the documentation saying to use MCSampler with the rule='latin_hypercube' option to get the same results as QMCSampler?

The following will also need to be updated/dropped:

grep QMCSampler tutorials/*.py tutorials/*.ipynb tests/*.py

tutorials/mega_tutorial.ipynb:    "qmc_sampler = uq.sampling.QMCSampler(vary, 128)"

tutorials/vector_qoi_tutorial.ipynb:    "For this tutorial we will use Polynomial Chaos Expansion method. However, both [SCSampler](https://easyvvuq.readthedocs.io/en/dev/easyvvuq.sampling.html#module-easyvvuq.sampling.stochastic_collocation) and [QMCSampler](https://easyvvuq.readthedocs.io/en/dev/easyvvuq.sampling.html#module-easyvvuq.sampling.qmc) would work as well and might be preferable depending on the case."

tests/test_analysis_qmc_analysis.py:from easyvvuq.sampling.qmc import QMCSampler
tests/test_analysis_qmc_analysis.py:    sampler = QMCSampler(vary, 100)

tests/test_integration.py:#     sampler = uq.sampling.QMCSampler(vary=vary,

tests/test_sampling_qmc.py:from easyvvuq.sampling import QMCSampler
tests/test_sampling_qmc.py:        QMCSampler({}, 100)
tests/test_sampling_qmc.py:        QMCSampler([], 100)
tests/test_sampling_qmc.py:    sampler = QMCSampler(vary, 100)
tests/test_sampling_qmc.py:    sampler = QMCSampler(vary, 100)
tests/test_sampling_qmc.py:    sampler = QMCSampler(vary, 100, 390)

@orbitfold
Copy link
Collaborator

The less code in the repository the better.

@djgroen
Copy link
Contributor

djgroen commented Jun 7, 2022

Hi all. I'm strongly in favour of this merge too, so feel free to go ahead with it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants