search

choderalab / chiron

Differentiable Markov Chain Monte Carlo
https://github.com/choderalab/chiron/wiki
MIT License
14 stars 1 forks source link

Refactoring of MC moves #21

Closed wiederm closed 5 months ago

wiederm commented 7 months ago

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

chrisiacovella commented 6 months ago

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 commented 6 months ago

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

codecov-commenter commented 6 months ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (multistage@ccdc4cb). Click here to learn what that means. The diff coverage is 86.90%.

Additional details and impacted files
chrisiacovella commented 6 months ago

CI is now up and running and are now all passing.

Note, I had to comment out the match-case syntax in multistate.py as that isn't supported in python 3.9.

chrisiacovella commented 6 months ago

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.