Disable improved sampling for nm_mcsolve

[pmenczel]

Description
The improved_sampling option was recently added to the MCSolver. NonMarkovianMCSolver extends MCSolver and can therefore be called with improved_sampling=True. Without this PR, it would then silently return wrong results.

The technical reason is that nm_mcsolve calculates the value of an influence martingale, which is factored into the calculation of expectation values in the custom result class NmmcResult. If improved_sampling is enabled, McResultImprovedSampling is used instead.

I here only disable the improved_sampling option for nm_mcsolve and print a warning if a user tries to use it. I have created a new issue to discuss ways to implement the improved sampling for nm_mcsolve.

Related issue
#2235

[pmenczel]

I now noticed that the tests failed because of the added warnings. Unsure how to handle this, could add @pytest.mark.filterwarnings('ignore::UserWarning') to the tests?

[Ericgig]

You should remove the options from solver_options instead.
It contain every valid options keys, just removing the key from it would makes setting it an error.

    solver_options = {
        **MCSolver.solver_options,
        "completeness_rtol": 1e-5,
        "completeness_atol": 1e-8,
        "martingale_quad_limit": 100,
    }
    del solver_options["improved_sampling"]

Also I would suggest to overwrite the options property to add the completeness_rtol, etc. to it's docsting.

[pmenczel]

Hi Eric, thanks for your comment.

You should remove the options from solver_options instead. It contain every valid options keys, just removing the key from it would makes setting it an error.

Unfortunately it is not so easy, since MCSolver relies on the option being present in self.options:

qutip/qutip/solver/mcsolve.py

Line 474 in 48df19c

if not self.options["improved_sampling"]:

We could try overriding every method of MCSolver that accesses this option, but that seems easy to break accidentally again with future changes in MCSolver.

Also I would suggest to overwrite the options property to add the completeness_rtol, etc. to it's docsting.

Done.

[pmenczel]

Note: I tested the
del solver_options["improved_sampling"]
in this branch in my fork: https://github.com/pmenczel/qutip/tree/nmmcsolve-delete-sampling-option

[Ericgig]

solver_options contains all available options. I don't think we want to breaks this pattern for this.
Maybe fixing MCSolver to use options.get("improved_sampling", False) instead of options["improved_sampling"] would be enough for now.

But it shows that our solver class structure is to be reworked or we need an equivalent to improved_sampling for nm_mcsolve.

[pmenczel]

solver_options contains all available options. I don't think we want to breaks this pattern for this. Maybe fixing MCSolver to use options.get("improved_sampling", False) instead of options["improved_sampling"] would be enough for now.

Okay, I have fixed it like this for now.

But it shows that our solver class structure is to be reworked or we need an equivalent to improved_sampling for nm_mcsolve.

I could try implementing improved_sampling for nm_mcsolve, but that would need a bit more time.

[Ericgig]

Happy with that.