Multistate sampling #8
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wiederm
added this to the Perform free energy calculation of HO at different thermodynamic states milestone
Implementation of offline free energy estimator
* Add MBAREstimator class and MultistateReporter class * Add MBAR class and update free energy estimators * Update LangevinIntegrator class in integrators.py * Update MCMCMove and LangevinDynamicsMove constructors * Fix MBAREstimator initialization in MultiStateSampler * Refactor reporters and tests * Add new reporters and update tests * Wrap and rebuild neighborlist in LangevinIntegrator * Refactor code to transpose u_kn array in MultiStateSampler and _SimulationReporter * Refactor _SimulationReporter class to improve code readability and maintainability * Refactor code and add random seed functionality * Fix reporter visibility and add test for multistate reporter
* Refactor MCMC sampler and MultiStateSampler classes * Refactor MCMC and MultiStateSampler classes
This was referenced Feb 8, 2024
* Fix u_kn shape in test_multistate_run * Refactor MCMCMove subclasses and update method names * Started MC refactor, merging from multistage branch that includes new reporters and random number scheme. This primarily sketches out the new MCMove class (updates to the actual moves is forthcoming). * Small refactor to Langevin integrator. Small changes: run command now takes variable to allow for velocity initialization. The Langevin ntegrator now returns a sampler state rather tha updating. States now has a velocity setter. * created initialize_velocities funtion in utilities; langevin code now 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. * Finished fixing up langevin integrator/move * Implemented Metropolis displacement move in new scheme. * Added refactored barostat move and ideal gas test case. * Abstractmethod for _reporting wrapper (this will make it easier for each move to have custom/appropriate logging for what is being changed). * Added _reporter to the displacement moves and the ability to only move a subset of atoms: tests need to be updated/implemented still. * Added in stepsize updater to allow move_parameters to be updated on the fly. * Continued reworking to ensure expected behavior of loggers and stepsize adjustments. * Fixed test_integrator and atom_subsetting in displacement move. * Added in flag to initialize velocities the first time langevin is called. 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. * test_multistate still is failing an assertion, but I think that is fine, 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 more with the functional programming model (passing and return a neighborlist). * Added ideal gas test written in the other PR. updated logic in langevin to take into account the iteration not just the current step; reporters now also log the elapsed_step. * Added ideal gas test written in the other PR. updated logic in langevin 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 * Fixed convergence test syntax: these were missed on my part because they were marked to be skipped because they take too long. * Fixed convergence test syntax: these were missed on my part because they were marked to be skipped because they take too long. for Harmonic oscillator we do not seem to need to run as long as set initially to pass and get convergence. * Updated CI.yaml to enabled CI to run for branches commiting to the multistage branch. Thanks Mike Henry! * Added in skip to the multistate testing, as this still needs to be worked on in the multistate branch this is being commited to" * match/case statements don't exist in python 3.9. I commented this out and just added in if/elif statements. * fixture was missing in test_testsystems * fixture was missing in test_testsystems * weirdly wrong syntax in the test ideal gas test. * Updating ideal gas example. * Update Examples/Idealgas.py with descriptive assert statement Co-authored-by: Marcus Wieder <[email protected]> * Update Examples/Idealgas.py with descriptive assert statement Co-authored-by: Marcus Wieder <[email protected]> * Updating ideal gas example. * Updating ideal gas example. * removed velocity initialization flag in langevin * updated various functions in reponse to marcus' comments. * Merge failed to correctly merge, and broke MCMCSampler; fixed now. multistate reporter giving an error. * Working through comments from Jchodera. * Fixed error with multistate systems. Various other updates based on comments. * Changed `coordinates` to `positions` in neighbor/pairlist routines to be consistent with samplerstate variable name change. * Changed PairListNsqrd to allow setting the cutoff to None which will 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. * mised an instance where the reporter called space.box_vectors. fixed. * name refactoring in MCMC * further addressing comments. * Added accumulated for number_of_attemps_made instead of elapsed_steps. (I missed this comment in the PR comments) --------- Co-authored-by: chrisiacovella <[email protected]>
chrisiacovella
requested changes
Feb 29, 2024
…one for each sampler state). Changed test system (HOA) to use nonperiodic space and the pair list; modified neighbor/pairlist classes to not fail if box_vectors = None (errors will be thrown in Space class if box vectors are needed).
|
I did a few commits where I made the multistate code support accepting a list of neighbor/pair lists (this required some changes to the neighbor/pair list logic itself, as they weren't properly supporting systems without box_vectors set). I'll note that the harmonic oscillator tests don't actually use the neighbor/pair list at all (not supported in the potential, for obviously reasons), so I think we might still need some additional functionality tests to make sure these are used correctly. |
|
I will create a separate Example of the multistate harmonic oscillator (basically a standalone copy of the code in test_multistate). This can explore the number of steps to converge to the analytical values of the free energy to make sure the the number of steps actually are sufficient. |
chrisiacovella
approved these changes
Mar 22, 2024
There was a problem hiding this comment.
I've addressed my comments I added and created a few issues so we don't miss them.
I think we should probably drop support for 3.9 soon, so we can use future annotations without issue; I think we will need to try out 3.12 (not sure if all our dependencies are supported yet).
|
Thank you for taking this over @chrisiacovella ! Great work! I agree that we can drop 3.9 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Simulate multiple (thermodynamic) states. This base implementation provides multistate sampling, assuming a single replicate and sampler state per thermodynamic state. It also implements these methods for sequential computation.
This implementation uses code from
openmmtoolswhere possible.Todos
Status