UCL-CCS / EasyVVUQ

Python 3 framework to facilitate verification, validation and uncertainty quantification (VVUQ) for a wide variety of simulations.
https://easyvvuq.readthedocs.io/
GNU Lesser General Public License v3.0
85 stars 27 forks source link

We could merge the QMCSampler and MC sampler #372

Open wedeling opened 2 years ago

wedeling commented 2 years ago

The QMCSampler does not work with DiscreteUniform inputs, and I see no apparent way of fixing this, see https://github.com/UCL-CCS/EasyVVUQ/issues/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 commented 2 years ago

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 commented 2 years ago

The less code in the repository the better.

djgroen commented 2 years ago

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