Improved sampling algorithm for mcsolve #2218

dkweiss31 · 2023-08-21T16:53:26Z

Description
Added functionality associated with an improved monte-carlo sampling algorithm described in https://journals.aps.org/pra/abstract/10.1103/PhysRevA.99.052327 . This algorithm samples the no-jump trajectory only once, then only samples jump trajectories afterwards. Expectation values, final states, etc are then computed by appropriately weighting the no jump and jump trajectories by the no-jump probability.

… to perturb default functionality. Also ensure state averages calculated correctly

…solve tests

coveralls · 2023-08-21T18:34:07Z

coverage: 85.002% (+0.1%) from 84.88% when pulling 966f495 on dkweiss31:mcsolve_efficienvy_v2 into beacadf on qutip:master.

Ericgig

Thank you for the contribution, this seems an interesting variant the the Monte-carlo algorithm.

Should it raise a warning for users that tries to access each trajectories? Or add a note in the docstring? The trajectories would look strange if you don't read the documentation.

I am wondering if MCSolverImprovedSampling could be merged in MCSolver. I am not a fan of having mcsolve use 2 different solvers and it should be accessing by the new class interface. Another options would be that make MCSolverImprovedSampling public and add a new function to call it mcissolve?

I would prefer that you revert the changes made by black. It add a lot of changes that are unrelated to the new feature and makes reviewing the PR annoying. Also we use the pep8 line width of 80 instead of black's 88.
For now, we mostly use black in new code and adding black to existing file should be their own PR.

qutip/solver/multitraj.py

qutip/solver/result.py

qutip/tests/solver/test_mcsolve.py

Ericgig · 2023-08-21T19:18:30Z

qutip/solver/mcsolve.py

+    seeds=None,
+    target_tol=None,
+    timeout=None,
+    improved_sampling=False,


I feel this should be in options. options include modification to the algorithm that do not change the physic.

Right so I guess the question is: do we make a new function mcissolve or add this as a flag to options. Maybe I'd be more inclined for a new function, but I don't have a strong preference

This reverts commit f034705

dkweiss31 · 2023-08-21T21:15:23Z

Thanks for the quick reply!

Should it raise a warning for users that tries to access each trajectories? Or add a note in the docstring? The trajectories would look strange if you don't read the documentation.

Maybe, though I'm not sure why they would look strange? It's possible a user could get confused if they don't see any no-jump trajectories other than the first one, but hopefully they wouldn't be confused if they set the flag to use this algorithm.

I am wondering if MCSolverImprovedSampling could be merged in MCSolver. I am not a fan of having mcsolve use 2 different solvers and it should be accessing by the new class interface. Another options would be that make MCSolverImprovedSampling public and add a new function to call it mcissolve?

Yeah this might be the way to go. I only had it the way I did to have MultiTrajSolverImprovedSampling get inherited before MCSolver so that the functionality there gets used and also have resultclass = McResultImprovedSampling. We could do a conditional inheritance in MCSolver, but I'm worried that might be messy/ugly?

I would prefer that you revert the changes made by black. It add a lot of changes that are unrelated to the new feature and makes reviewing the PR annoying. Also we use the pep8 line width of 80 instead of black's 88. For now, we mostly use black in new code and adding black to existing file should be their own PR.

Sorry about that, will revert

…istent line breaking (80), check for self.num_trajectories==1 instead of a no-jump flag since the no jump is always first

Ericgig

I believe that MultiTrajSolverImprovedSampling and MultiTrajResultImprovedSampling. We only need one new class for the solver and result, not 2 each.
It would even be nice if it could be inserted into MCSolver without making the code to messy...

I believe that a scaling should be applied in to the McResult.photocurrent method. Could you check. It's an histogram of the jumps times.

The last issue is whether to make a new function or not.
I feel it belong as an options in the existing mscolve, but it would be easy to overlook by users. As a new function it will get more attention. I also don't have a strong opinion on the matter.

qutip/solver/result.py

qutip/solver/multitraj.py

… into MCSolver, eliminated MultiTrajSolverImprovedSampling and moved that functionality into MCSolver, MultiTrajResultImprovedSampling -> McResultImprovedSampling, fixed photocurrent computation, fixed tests (mostly adding improved_sampling as a key in options, some pep8 stuff)

…sampling

dkweiss31 · 2023-08-24T19:51:52Z

I believe that MultiTrajSolverImprovedSampling and MultiTrajResultImprovedSampling. We only need one new class for the solver and result, not 2 each. It would even be nice if it could be inserted into MCSolver without making the code to messy...

Agreed

I believe that a scaling should be applied in to the McResult.photocurrent method. Could you check. It's an histogram of the jumps times.

Yes you're right, sorry I missed that. Should be fixed now.

The last issue is whether to make a new function or not. I feel it belong as an options in the existing mscolve, but it would be easy to overlook by users. As a new function it will get more attention. I also don't have a strong opinion on the matter.

I think you are right. I added it as an option to options and updated tests, docs accordingly.

dkweiss31 · 2023-08-24T19:59:22Z

qutip/solver/mcsolve.py

@@ -385,6 +408,9 @@ def __init__(self, H, c_ops, *, options=None):
                rhs -= 0.5 * n_op

        self._num_collapse = len(self._c_ops)
+        self.options = options
+        if self.options["improved_sampling"]:
+            self.resultclass = McResultImprovedSampling


This is something I'm not super happy about: this wants to be defined outside of init (where we have resultclass = McResult now). Maybe still not a good enough reason to define a whole new class MCSolverImprovedSampling though

Or maybe set resultclass to be a property that check the option and return the right type.

Another options would by using a metaclass. It feels overkill, but would be clean (here at least).

The metaclass idea sounds fun but maybe the below simple fix works?

Ericgig

Looking good.

I left some ideas on how to improve the resultclass setting.

doc/guide/dynamics/dynamics-monte.rst

Ericgig · 2023-08-24T20:35:20Z

qutip/solver/mcsolve.py

@@ -385,6 +408,9 @@ def __init__(self, H, c_ops, *, options=None):
                rhs -= 0.5 * n_op

        self._num_collapse = len(self._c_ops)
+        self.options = options
+        if self.options["improved_sampling"]:
+            self.resultclass = McResultImprovedSampling


Or maybe set resultclass to be a property that check the option and return the right type.

Another options would by using a metaclass. It feels overkill, but would be clean (here at least).

doc/guide/dynamics/dynamics-monte.rst

qutip/solver/result.py

Co-authored-by: Eric Giguère <eric.giguere@calculquebec.ca>

…mproved_sampling"]

…lve_efficienvy_v2

Ericgig

Thank you @dkweiss31.
Everything looks fine to me.

dkweiss31 added 6 commits August 20, 2023 22:21

improved sampling algorithm for mcsolve working in initial tests

499d44d

avoid code duplication by utilizing existing classes, taking care not…

9101b74

… to perturb default functionality. Also ensure state averages calculated correctly

black

f034705

small bug fixes in average_states, added tests leveraging existing mc…

124eabc

…solve tests

docstrings, updated user guide

5284162

small additions to docs

1fff804

dkweiss31 changed the title ~~Mcsolve efficienvy v2~~ Improved Sampling algorithm for mcsolve Aug 21, 2023

dkweiss31 changed the title ~~Improved Sampling algorithm for mcsolve~~ Improved sampling algorithm for mcsolve Aug 21, 2023

dkweiss31 and others added 2 commits August 21, 2023 12:56

added towncrier changelog

f29d6b8

Merge branch 'qutip:master' into mcsolve_efficienvy_v2

966f495

Ericgig reviewed Aug 21, 2023

View reviewed changes

dkweiss31 added 2 commits August 21, 2023 16:29

better parametrization of tests

8ea0c56

Revert "black"

be9a6e9

This reverts commit f034705

dkweiss31 added 2 commits August 21, 2023 17:37

eliminated unnecesary function in MCSolverImprovedSampling, more cons…

3a43fd0

…istent line breaking (80), check for self.num_trajectories==1 instead of a no-jump flag since the no jump is always first

added public classes to __all__, ensured compliance with pycodestyle

9dc74d0

Ericgig reviewed Aug 24, 2023

View reviewed changes

qutip/solver/result.py Outdated Show resolved Hide resolved

qutip/solver/multitraj.py Outdated Show resolved Hide resolved

qutip/solver/multitraj.py Outdated Show resolved Hide resolved

dkweiss31 added 2 commits August 24, 2023 14:42

updated documentation with an example plot showing usage of improved_…

7e181d5

…sampling

small docs bug fix

7198a45

dkweiss31 commented Aug 24, 2023

View reviewed changes

Ericgig reviewed Aug 24, 2023

View reviewed changes

qutip/solver/result.py Outdated Show resolved Hide resolved

dkweiss31 and others added 5 commits August 24, 2023 20:44

Update doc/guide/dynamics/dynamics-monte.rst

b38134e

Co-authored-by: Eric Giguère <eric.giguere@calculquebec.ca>

Update qutip/solver/result.py

96d7ad6

Co-authored-by: Eric Giguère <eric.giguere@calculquebec.ca>

using decorator @Property to set resultclass based on self.options["i…

f18e078

…mproved_sampling"]

Merge remote-tracking branch 'origin/mcsolve_efficienvy_v2' into mcso…

851d7e6

…lve_efficienvy_v2

small clarity changes to docs

2984036

Ericgig approved these changes Aug 25, 2023

View reviewed changes

Ericgig merged commit 2dc5f3f into qutip:master Aug 25, 2023
12 checks passed

dkweiss31 deleted the mcsolve_efficienvy_v2 branch August 25, 2023 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved sampling algorithm for mcsolve #2218

Improved sampling algorithm for mcsolve #2218

dkweiss31 commented Aug 21, 2023

coveralls commented Aug 21, 2023

Ericgig left a comment

Ericgig Aug 21, 2023

dkweiss31 Aug 21, 2023

dkweiss31 commented Aug 21, 2023

Ericgig left a comment

dkweiss31 commented Aug 24, 2023

dkweiss31 Aug 24, 2023

Ericgig Aug 24, 2023

dkweiss31 Aug 25, 2023

Ericgig left a comment

Ericgig Aug 24, 2023

Ericgig left a comment

Improved sampling algorithm for mcsolve #2218

Improved sampling algorithm for mcsolve #2218

Conversation

dkweiss31 commented Aug 21, 2023

coveralls commented Aug 21, 2023

Ericgig left a comment

Choose a reason for hiding this comment

Ericgig Aug 21, 2023

Choose a reason for hiding this comment

dkweiss31 Aug 21, 2023

Choose a reason for hiding this comment

dkweiss31 commented Aug 21, 2023

Ericgig left a comment

Choose a reason for hiding this comment

dkweiss31 commented Aug 24, 2023

dkweiss31 Aug 24, 2023

Choose a reason for hiding this comment

Ericgig Aug 24, 2023

Choose a reason for hiding this comment

dkweiss31 Aug 25, 2023

Choose a reason for hiding this comment

Ericgig left a comment

Choose a reason for hiding this comment

Ericgig Aug 24, 2023

Choose a reason for hiding this comment

Ericgig left a comment

Choose a reason for hiding this comment