Skip to content
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.

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

Restraint forces #336

Merged
merged 29 commits into from
Mar 22, 2018
Merged

Restraint forces #336

merged 29 commits into from
Mar 22, 2018

Conversation

andrrizzi
Copy link
Contributor

The main thing in this PR is the addition in the forces.py module of restraint forces as anticipated in #328. These forces provide a common interface that will allow us in YANK's analysis module to reweight radially-symmetric restraints without implementing non-extensible hacks to figure out the restraints properties (cc choderalab/yank#882). The most important feature introduced is the ability to compute the standard state correction with an energy/radius cutoff and reweighting to a square-well potential.

Other smaller things in this PR:

Do I have to manually update the list of classes in the docs or are they automatically generated?

@jchodera
Copy link
Member

@andrrizzi : Is this PR ready for review?

@andrrizzi
Copy link
Contributor Author

Yep!

@jchodera jchodera requested a review from Lnaden March 21, 2018 23:34
@jchodera
Copy link
Member

Do I have to manually update the list of classes in the docs or are they automatically generated?

This is a good question for @Lnaden, but I think you have to manually update the list of classes.

@Lnaden : Can you review when you have a chance?

Copy link
Contributor

@Lnaden Lnaden left a comment

Choose a reason for hiding this comment

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

This is great, will this wind up replacing some of the Restraint classes in yank eventually?

@Lnaden
Copy link
Contributor

Lnaden commented Mar 22, 2018

Do I have to manually update the list of classes in the docs or are they automatically generated?

If you have the autodoc extension included, it should compile if you also specify in the docs where you want them built. But if you allow it to just do its thing with minimal settings, it will be a bit of a cudgel and document everything it finds where you target it.

@andrrizzi
Copy link
Contributor Author

This is great, will this wind up replacing some of the Restraint classes in yank eventually?

I don't think so. I think the classes in YANK will keep serving the role of restraint factories that create these force objects, add automatic determination of the parameters, and parse DSL/SMIRKS strings (through the Topography object).

If you have the autodoc extension included, it should compile if you also specify in the docs where you want them built. But if you allow it to just do its thing with minimal settings, it will be a bit of a cudgel and document everything it finds where you target it.

Ok, thanks! I'll merge this, see what gets generated automatically, and finish up the docs update in a separate PR.

@andrrizzi andrrizzi merged commit 54afdda into master Mar 22, 2018
@dwhswenson
Copy link
Collaborator

This PR seems to have removed the generic RestorableIntegrator class introduced in #140 ... should we now only refer to ThermostatedIntegrator? Will it work the same? (Especially the methods is_restorable and restore_interface). The most recent release of openmmtools is causing test failures for OpenPathSampling.

@jchodera
Copy link
Member

I believe that is correct. Apologies for the lack of advance warning!

OpenMM added support for serializing/deserializing Python subclasses of CustomIntegrator in 7.2.0, so this feature was no longer needed, but we didn't anticipate this change would break other code.

@andrrizzi : What do you think about creating a bugfix release that restores RestorableIntegrator as a simple subclass of CustomIntegrator so that existing code will continue to work?

@dwhswenson
Copy link
Collaborator

I think that the extent of our use is:

https://github.com/openpathsampling/openpathsampling/blob/master/openpathsampling/engines/openmm/engine.py#L14-L40

If there's now a better way to accomplish that, based on pure OpenMM, it would probably be better for us to do that (at least, long term). Will simtk.openmm.XmlSerializer.deserialize(integrator_xml) now give us the full integrator interface? Or is there something else we should use?

In any case, we might keep a variant of the function linked above to support older versions of OpenMM. But even that looks like it might be a 1-line fix, so if we're the only ones suffering the problem, it'll be as easy to fix it on our end as for you to push a bugfix.

Apologies for the lack of advance warning!

No worries -- API changes are expected when we're using a version 0.x!

@andrrizzi
Copy link
Contributor Author

Apologies for the change. The RestorableIntegrator functionalities are still there, but I've moved them into openmmtools.utils.RestorableOpenMMObject which is capable of doing the same thing also with Forces. You can also use ThermostateIntegrator if you prefer since it inherits from RestorableOpenMMObject.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants