8000 `LangevinDynamicsMove` now accepts `constraint_tolerance` parameter by ijpulidos · Pull Request #611 · choderalab/openmmtools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

LangevinDynamicsMove now accepts constraint_tolerance parameter #611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Aug 2, 2022
Merged

LangevinDynamicsMove now accepts constraint_tolerance parameter #611

merged 6 commits into from
Aug 2, 2022

Conversation

ijpulidos
Copy link
Contributor
@ijpulidos ijpulidos commented Jul 27, 2022 •
edited
Loading

Description

LangevinDynamicsMove now accepts a constraint_tolerance parameter (defaults to the same value as the previously used LangevinIntegrator).

Resolves #608

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelog to summarize changes in behavior, enhancements, and bugfixes implemented in this PR

Status

  • Ready to go

Changelog

``LangevinDynamicsMove`` now allows ``constraint_tolerance`` parameter and public attribute, for specifying the fraction of the constrained distance within which constraints are maintained for the integrator (Refer to `Openmm's documentation <http://docs.openmm.org/latest/api-python/generated/openmm.openmm.LangevinMiddleIntegrator.html#openmm.openmm.LangevinMiddleIntegrator.setConstraintTolerance>_ for more information).

@ijpulidos ijpulidos added this to the 0.21.5 milestone Jul 27, 2022
mikemhenry
mikemhenry requested changes Jul 27, 2022
View reviewed changes
@mikemhenry
Copy link
Contributor

Also maybe we should add to the changelog entry a link to the openmm doc for that setting.

@zhang-ivy
Copy link
Contributor
zhang-ivy commented Jul 27, 2022 •
edited
Loading

Looks great, thanks!! And I agree with Mike's suggestions -- especially the one about adding a link to the openmm doc. Not completely sure if the deserialization thing is necessary but seems like it would be useful.

@mikemhenry
Copy link
Contributor
mikemhenry commented Jul 28, 2022 •
edited
Loading

Serialization will be important if you want to load an nc simulated with an older version of openmmtools but want to look at it after this version comes out, see #612 for an example where we missed this.

@zhang-ivy
Copy link
Contributor

Yes I definitely agree it would be useful, but I just meant I’ll leave it up to you guys to figure out what you want to do

@mikemhenry mikemhenry self-requested a review August 1, 2022 19:58
mikemhenry
mikemhenry requested changes Aug 1, 2022
View reviewed changes
Copy link
Contributor
@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidently dismised my review.

The only blocker on this is that we need to handle the case where constraint_tolerance isn't in the nc file.

@ijpulidos
Copy link
Contributor Author
ijpulidos commented Aug 2, 2022 •
edited
Loading

The only blocker on this is that we need to handle the case where constraint_tolerance isn't in the nc file.

@mikemhenry I just checked and it isn't a problem, to my surprise. I was kind of expecting for it to be a problem, so it took me a while to realize why I think it isn't. As far as I can see, the flow of the object when deserializing is something like the following.

  1. The sampler gets created from the nc files with the from_storage method.
  2. This method then creates the instance of the sampler with the sampler's _instantiate_sampler_from_reporter method
  3. The previous step generates/deserialize instances of the mcmc_moves using the reporter's read_mcmc_moves method
  4. Lastly, after calling the utility deserializing function, deserialize, this function just creates new instances of the mcmc_moves objects by calling their respective __init__ via the __new__ method here, since we are not overloading __new__ in the MCMCmove objects. Resulting in using the attributes that are serialized, falling back to defaults for those who were not serialized.

I hope that's understandable. Let me know if I am making myself clear here.

The good news is that in #613 we are also indirectly testing this, since we are using legacy/older versions of serialized systems in the test. But I don't know if we want a specific test for this checking the match between serialized attributes vs. the ones that are falling back to default when deserializing MCMCMove objects from an older simulation. Generally speaking, we don't really have many tests for resuming simulations, as far as I can tell.

@ijpulidos ijpulidos requested a review from mikemhenry August 2, 2022 03:45
@mikemhenry
Copy link
Contributor

The good news is that in #613 we are also indirectly testing this, since we are using legacy/older versions of serialized systems in the test.

This is great since it will check for regressions, I want to then get 613 merged in first so we can have that check for this PR.

@zhang-ivy
Copy link
Contributor

@mikemhenry @ijpulidos : I merged #613 into main and then merged main into this branch and then double checked that TestSerializedMultiStateSampler (introduced in #613) is passing successfully in the latest GHA workflows. Are you ok with me merging this now?

@mikemhenry
Copy link
Contributor

Sounds good, we can have John review the release PR

@zhang-ivy
Copy link
Contributor

@mikemhenry Can you "approve" this PR? merging is blocked because you requested changes

@mikemhenry mikemhenry enabled auto-merge (squash) August 2, 2022 20:10
@mikemhenry mikemhenry merged commit ea3f9c6 into main Aug 2, 2022
@mikemhenry mikemhenry deleted the 608-langevindynmove-tolerance branch August 2, 2022 20:19
@zhang-ivy zhang-ivy mentioned this pull request Aug 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikemhenry mikemhenry mikemhenry approved these changes

@zhang-ivy zhang-ivy zhang-ivy approved these changes

@jchodera jchodera Awaiting requested review from jchodera

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.21.5
Development

Successfully merging this pull request may close these issues.

Allow integrator tolerance to be specified for mcmc.LangevinDynamicsMove
3 participants
@ijpulidos @mikemhenry @zhang-ivy
0