Refactoring of MC moves #21

wiederm · 2024-01-18T15:58:53Z

Description

Currently, we have MC moves designed for displacement proposals, implementing the proposal in the MetropolisedDisplacementMove class that inherits from MetropolizedMove, which implements the computation for the acceptance criteria (proposal probability).
This lacks flexibility for other types of MC moves with different calculations for the acceptance criteria (like a MC Barostate).

Here, we want to consolidate the different approaches

Todos

[ ]

Status

Ready to go

# Conflicts: # chiron/mcmc.py # chiron/tests/test_multistate.py

… reporters and random number scheme. This primarily sketches out the new MCMove class (updates to the actual moves is forthcoming).

# Conflicts: # chiron/mcmc.py # chiron/tests/test_multistate.py

… takes variable to allow for velocity initialization. The Langevin ntegrator now returns a sampler state rather tha updating. States now has a velocity setter.

… uses this if users request velocities to be regenerated each time. The reaosn to split was to make it easy to define velocities outside of the langevin integrator. the option flag to generate each time we call the integrator because in a hybrid workflow, there would effectively be no connection between subsequent langevin moves separated by many MC transformations. furthermore, MC moves that changes thermodynamic states would require regeneration.

…ach move to have custom/appropriate logging for what is being changed).

…e a subset of atoms: tests need to be updated/implemented still.

…he fly.

…ze adjustments.

…led. This is different than reinitialize which will call reinit every time run is called. The velocity init function was spun off into the utils file. velocities can be set in the sampler state; code will throw and error if initialize_velocities or reinitialize_velocities are false, and velocities are not set in the sampler state.

…ne, since this PR was focused on revamping the MC moves. That can be tackled separately in the multistate PR.

…e with the functional programming model (passing and return a neighborlist).

…in to take into account the iteration not just the current step; reporters now also log the elapsed_step.

chrisiacovella · 2024-01-31T20:10:48Z

I think this is ready to go. The meat of this has been discussed in issue #20. I'll note that following #23 we no longer modify the sampler_state, thermodynamic_state and neighbor_list in place. These are now returned by functions.

This should make it much easier to keep track of current/proposed states, rather than trying reset/rollback changes made to a sampler_state. The one caveat is having to make sure we update the current_PRNG_key if we reject a state so we don't end up just proposing the same move over and over again. This is done in code that is in the base class, so users extending won't have to think about that.

For book keeping, note that this incorporates the MC barostat that was implemented in #14 , but uses the modified API.

chrisiacovella · 2024-01-31T20:11:25Z

@wiederm Since you opened this, I can't suggest you as a reviewer, but could you review it?

…in to take into account the iteration not just the current step; reporters now also log the elapsed_step. Updated the examples

…hey were marked to be skipped because they take too long.

wiederm

Very happy with these changes! I left a view comments, but I think we can merge this!

.github/workflows/CI.yaml

Examples/Idealgas.py

wiederm · 2024-02-08T09:49:31Z

Examples/Idealgas.py

+n_particles = 216
+temperature = 298 * unit.kelvin
+pressure = 1 * unit.atmosphere
+mass = unit.Quantity(39.9, unit.gram / unit.mole)


Is there a way to obtain the mass from the IdealGas object to minimize our responsibility for ensuring that the mass is set to the same value?

It seems the topology instance of the ideal_gas instance should have this information.

Yes. Good catch.

I'm wondering if it would be good to have examples be self contained (i.e., not require openmmtools). I think the openmmtools dependence is definitely good for tests/unit tests. It would definitely not be hard to have some simple routines for initializing particles. I don't think we need to do that at this time.

Examples/Idealgas.py

chiron/mcmc.py

wiederm · 2024-02-08T15:19:15Z

chiron/mcmc.py

+            proposed_sampler_state, proposed_nbr_list
+        )
+
+        #  ⎡−β (ΔU + PΔV ) + N ln(V new /V old )⎤


we might want to give a reference for this, e.g.
eq.33, http://dx.doi.org/10.1155/2015/183918

Or we could go back to the original source: https://www.tandfonline.com/doi/abs/10.1080/00268977200100031

chiron/mcmc.py

wiederm · 2024-02-08T15:21:19Z

chiron/mcmc.py

+        current_reduced_pot : float, required
+            Current reduced potential.


It might be easier if we are explicit here and add energy.

See my prior comment. I don't think we should add energy to the name.

wiederm · 2024-02-08T15:23:32Z

chiron/mcmc.py

+    def _update_stepsize(self):
+        """
+        Update the volume_max_scale parameter to ensure our acceptance probability is within the range of 0.25 to 0.75.
+        The maximum volume_max_scale will be capped at 0.3.
+        """
+        acceptance_ratio = self.n_accepted / self.n_proposed
+        if acceptance_ratio < 0.25:
+            self.volume_max_scale /= 1.1
+        elif acceptance_ratio > 0.75:
+            self.volume_max_scale = min(self.volume_max_scale * 1.1, 0.3)
+


Is there a way to refactor this into one of the base classes? This seems to be super helpful for general MC moves. But, it might also be specific enough so that abstracting it becomes difficult, then we can just leave it.

I defined it in the base class as an abstract method. I don’t think this will be general enough to put the algorithm in the base class (this is pretty specific to simple displacement and volume changes moves…would not apply to a insertion/deletion move). We could make this a class as I suggested in an issue #26 , that we pass to the init function to set up the protocol (and nothing is adjusted if not set).

In #26, we discussed creating something like an Autotune object that handles auto-tuning behavior. That could provide an API for auto-tuning operations that adapt parameters like these, and could eventually implement asymptotically consistent approaches to make sure that we don't cause issues with sampling.

Yes, I think creating an Autotune class will be a good idea, especially for anything more advanced. I think that will be easy to implement in a separate PR so we can better discuss the API we want.

Co-authored-by: Marcus Wieder <31651017+wiederm@users.noreply.github.com>

chrisiacovella

I think addressed all of the comments. I think we are ready to merge after @wiederm you have another look through.

…ltistate reporter giving an error.

chrisiacovella · 2024-02-10T06:57:03Z

The Multistate reporter is failing after syncing with the multistate branch. I'm ok with merging without fixing that here (and punting to the main multistate branch), but I'll try to look at it more closely tomorrow.

jchodera

Great progress, folks!

Here are some comments on API consistency that I identified in reading across the examples and implementations.

Examples/Idealgas.py

jchodera · 2024-02-10T18:26:32Z

Examples/Idealgas.py

+# initialize the displacement move
+mc_barostat_move = MonteCarloBarostatMove(
+    volume_max_scale=0.2,
+    nr_of_moves=10,


Could we standardize abbreviations or avoid them altogether, such as n_moves or number_of_moves? Or maybe we can even say n_attempts_per_step?

But what if we run out of bytes with such long names?! I think that is a great point; n_attempts_per_step would probably be very clear.

Edit: Since this will also apply to the langevin integrator move, we might not want to use the word "attempts" in the variable. number_of_moves might be more generally applicable.

The Langevin move is a special case of un-Metropolized move, where every step is accepted.
These moves are also distinct in that the Langevin move has "number of steps to take per move application" and a Monte Carlo barostat has a "number of volume adjustments to attempt per move application". In either case, something like n_steps and n_attempts is probably more appropriate for those move types.

jchodera · 2024-02-10T18:27:39Z

Examples/Idealgas.py

+    volume_max_scale=0.2,
+    nr_of_moves=10,
+    reporter=reporter,
+    update_stepsize=True,


Instead of update, we might want to refer to this as autotune.
frequency would refer to number of events per step, but interval or period refers to the number of steps between events.

Suggest:

update_stepsize=True -> autotune=True

update_stepsize_frequency=100 -> autotune_interval=100

This will be much clearer. I was struggling with coming up with a clear/general name for this.

I also updated the name of the internal class function to be self._autotune

I think it's fine to make self.autotune be public, since it is reasonable to allow a user to change the autotune behavior on the fly.

chiron/mcmc.py

jchodera · 2024-02-10T21:08:06Z

chiron/mcmc.py


-    def compute_acceptance_probability(
+            elapsed_step = i + self._move_iteration * self.nr_of_moves


I think this is a really bad idea, since self.nr_of_moves can be changed on the fly.
If we need a step accumulator for statistics or auto-tuning, let's actually use a step accumulator like self._number_of_attempts_made and simply increment it with each move attempt.

chiron/mcmc.py

jchodera · 2024-02-10T21:30:27Z

chiron/mcmc.py

+    def _update_stepsize(self):
+        """
+        Update the volume_max_scale parameter to ensure our acceptance probability is within the range of 0.25 to 0.75.
+        The maximum volume_max_scale will be capped at 0.3.
+        """
+        acceptance_ratio = self.n_accepted / self.n_proposed
+        if acceptance_ratio < 0.25:
+            self.volume_max_scale /= 1.1
+        elif acceptance_ratio > 0.75:
+            self.volume_max_scale = min(self.volume_max_scale * 1.1, 0.3)
+


In #26, we discussed creating something like an Autotune object that handles auto-tuning behavior. That could provide an API for auto-tuning operations that adapt parameters like these, and could eventually implement asymptotically consistent approaches to make sure that we don't cause issues with sampling.

…omments.

… be consistent with samplerstate variable name change.

…use no cutoff (i.e. all pairs interact). Revamped some internal tooling regarding how units are treated internally. Space was revampped such that it takes box_vectors as a argument rather than storing them internally (fewer copies of this floating around), and will help to ensure treating the class as static will not mean using the wrong box vectors accidentally.

chrisiacovella

I believe I have addressed all of the comments or made issues capturing the comments/discussion for instances where I believe these would be better addressed in separate PRs, as this is already getting quite large. These issues I created are #31, #32, #33, #34, #35.

…. (I missed this comment in the PR comments)

chrisiacovella

I think this is all ready to go; I had missed one the comments regarding the elapsed steps (got marked as outdated).

Fix u_kn shape in test_multistate_run

b313867

wiederm changed the base branch from main to multistage January 18, 2024 15:59

wiederm linked an issue Jan 18, 2024 that may be closed by this pull request

MC move refactoring #20

Closed

chrisiacovella mentioned this pull request Jan 18, 2024

MC move refactoring #20

Closed

wiederm and others added 9 commits January 18, 2024 22:36

Refactor MCMCMove subclasses and update method names

a9d9f16

Merge branch 'multistage' into mc

1ab9b75

Merge branch 'multistage' into mc

15ba2e8

# Conflicts: # chiron/mcmc.py # chiron/tests/test_multistate.py

Started MC refactor, merging from multistage branch that includes new…

4af534a

… reporters and random number scheme. This primarily sketches out the new MCMove class (updates to the actual moves is forthcoming).

Merge remote-tracking branch 'main_origin/mc' into mc

70b3997

# Conflicts: # chiron/mcmc.py # chiron/tests/test_multistate.py

Small refactor to Langevin integrator. Small changes: run command now…

53f0616

… takes variable to allow for velocity initialization. The Langevin ntegrator now returns a sampler state rather tha updating. States now has a velocity setter.

Finished fixing up langevin integrator/move

5e52df2

Implemented Metropolis displacement move in new scheme.

469071d

chrisiacovella self-assigned this Jan 25, 2024

chrisiacovella added 10 commits January 25, 2024 17:30

Added refactored barostat move and ideal gas test case.

a3ea44d

Abstractmethod for _reporting wrapper (this will make it easier for e…

4ad677e

…ach move to have custom/appropriate logging for what is being changed).

Added _reporter to the displacement moves and the ability to only mov…

9c4d47b

…e a subset of atoms: tests need to be updated/implemented still.

Added in stepsize updater to allow move_parameters to be updated on t…

b6d88cc

…he fly.

Continued reworking to ensure expected behavior of loggers and stepsi…

c5bce7a

…ze adjustments.

Fixed test_integrator and atom_subsetting in displacement move.

df93003

test_multistate still is failing an assertion, but I think that is fi…

beb0bdc

…ne, since this PR was focused on revamping the MC moves. That can be tackled separately in the multistate PR.

Modified the routines to ensure that passing the nbr_list fits in mor…

0b57c2b

…e with the functional programming model (passing and return a neighborlist).

Added ideal gas test written in the other PR. updated logic in langev…

85a9ae4

…in to take into account the iteration not just the current step; reporters now also log the elapsed_step.

chrisiacovella self-requested a review January 31, 2024 20:21

chrisiacovella added 3 commits January 31, 2024 12:36

Added ideal gas test written in the other PR. updated logic in langev…

67a9831

…in to take into account the iteration not just the current step; reporters now also log the elapsed_step. Updated the examples

Added in additional tests for barostat

ea1695a

Fixed convergence test syntax: these were missed on my part because t…

c992b04

…hey were marked to be skipped because they take too long.

chrisiacovella requested a review from jchodera February 2, 2024 22:44

wiederm commented Feb 8, 2024

View reviewed changes

wiederm mentioned this pull request Feb 8, 2024

Multistate sampling #8

Merged

3 tasks

chrisiacovella and others added 6 commits February 9, 2024 13:16

Updating ideal gas example.

030435c

Update Examples/Idealgas.py with descriptive assert statement

3ec2aa0

Co-authored-by: Marcus Wieder <31651017+wiederm@users.noreply.github.com>

Update Examples/Idealgas.py with descriptive assert statement

0987cf2

Co-authored-by: Marcus Wieder <31651017+wiederm@users.noreply.github.com>

Updating ideal gas example.

fc9ede3

Updating ideal gas example.

1daab8a

removed velocity initialization flag in langevin

e419169

This was linked to issues Feb 9, 2024

Remove copy and switch to return SamplerState #24

Open

Swichting from OO to functional paradigma #23

Closed

chrisiacovella added 2 commits February 9, 2024 15:07

updated various functions in reponse to marcus' comments.

9ad8a23

Merge branch 'multistage' into mc

9e000db

chrisiacovella self-requested a review February 9, 2024 23:19

chrisiacovella approved these changes Feb 10, 2024

View reviewed changes

Merge failed to correctly merge, and broke MCMCSampler; fixed now. mu…

60e0814

…ltistate reporter giving an error.

jchodera reviewed Feb 10, 2024

View reviewed changes

chrisiacovella mentioned this pull request Feb 12, 2024

Create Chiron TestSystem class #30

Open

chrisiacovella added 7 commits February 12, 2024 13:00

Working through comments from Jchodera.

e78992d

Fixed error with multistate systems. Various other updates based on c…

07fe368

…omments.

Changed coordinates to positions in neighbor/pairlist routines to…

6a2733b

… be consistent with samplerstate variable name change.

mised an instance where the reporter called space.box_vectors. fixed.

12d3b10

name refactoring in MCMC

47ab2cc

further addressing comments.

192604e

chrisiacovella approved these changes Feb 26, 2024

View reviewed changes

Added accumulated for number_of_attemps_made instead of elapsed_steps…

ba3817d

…. (I missed this comment in the PR comments)

chrisiacovella approved these changes Feb 27, 2024

View reviewed changes

wiederm merged commit 124e519 into multistage Feb 27, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of MC moves #21

Refactoring of MC moves #21

wiederm commented Jan 18, 2024 •

edited by chrisiacovella

Loading

chrisiacovella commented Jan 31, 2024 •

edited

Loading

chrisiacovella commented Jan 31, 2024

wiederm left a comment

wiederm Feb 8, 2024

chrisiacovella Feb 9, 2024

wiederm Feb 8, 2024

chrisiacovella Feb 9, 2024

wiederm Feb 8, 2024

chrisiacovella Feb 9, 2024

wiederm Feb 8, 2024

chrisiacovella Feb 9, 2024

jchodera Feb 10, 2024

chrisiacovella Feb 27, 2024

chrisiacovella left a comment

chrisiacovella commented Feb 10, 2024

jchodera left a comment

jchodera Feb 10, 2024

chrisiacovella Feb 12, 2024 •

edited

Loading

jchodera Feb 12, 2024

jchodera Feb 10, 2024

chrisiacovella Feb 12, 2024

chrisiacovella Feb 12, 2024

jchodera Feb 12, 2024

jchodera Feb 10, 2024

jchodera Feb 10, 2024

chrisiacovella left a comment

chrisiacovella left a comment

		current_reduced_pot : float, required
		Current reduced potential.


		def compute_acceptance_probability(
		elapsed_step = i + self._move_iteration * self.nr_of_moves

Refactoring of MC moves #21

Refactoring of MC moves #21

Conversation

wiederm commented Jan 18, 2024 • edited by chrisiacovella Loading

Description

Todos

Status

chrisiacovella commented Jan 31, 2024 • edited Loading

chrisiacovella commented Jan 31, 2024

wiederm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisiacovella left a comment

Choose a reason for hiding this comment

chrisiacovella commented Feb 10, 2024

jchodera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisiacovella Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisiacovella left a comment

Choose a reason for hiding this comment

chrisiacovella left a comment

Choose a reason for hiding this comment

wiederm commented Jan 18, 2024 •

edited by chrisiacovella

Loading

chrisiacovella commented Jan 31, 2024 •

edited

Loading

chrisiacovella Feb 12, 2024 •

edited

Loading