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

OpenMM 7.2 preview build: What should go in it? #1939

Closed
jchodera opened this issue Nov 24, 2017 · 50 comments
Closed

OpenMM 7.2 preview build: What should go in it? #1939

jchodera opened this issue Nov 24, 2017 · 50 comments

Comments

@jchodera
Copy link
Member

Besides the forcefield conversion, are there other open PRs or issues that should be folded into the 7.2 preview build?

Also tagging @Lnaden @andrrizzi who were still struggling with NaNs.

@peastman
Copy link
Member

Since no one has suggested anything else, shall we start moving forward with the beta? I don't think there are any other critical features for this release.

@andrrizzi
Copy link
Contributor

@peastman could you maybe take a look at #1940? That should be a very quick fix. I can open a PR myself if you prefer, but I think it would be good to have it in 7.2.

@pgrinaway
Copy link

Would it be difficult to include #1830 for debugging on the reference platform? I don't expect that there is much physical utility to it, but when creating Custom*Force -laden systems, it can be helpful to see if a particular term in the interactions is anomalously high.

@peastman
Copy link
Member

@peastman could you maybe take a look at #1940?

We can still fix bugs between the beta and the release. For this issue, I'm just concerned about new features. (And yes, if you want to send a PR that would be great!)

Would it be difficult to include #1830 for debugging on the reference platform?

Yes, and it isn't even really defined what it means. See the discussion on that issue. It needs design before we can think about implementing anything.

@andrrizzi
Copy link
Contributor

We can still fix bugs between the beta and the release. For this issue, I'm just concerned about new features. (And yes, if you want to send a PR that would be great!)

OK, will do, thanks!

@pgrinaway
Copy link

Yes, and it isn't even really defined what it means. See the discussion on that issue. It needs design before we can think about implementing anything.

Sorry, let me be a bit more specific. I was referring to per-interaction energies in the CustomNonbondedForce, which is pairwise. Since the potential energy is computed as a sum of those pairwise terms, it is sometimes helpful to see if some terms in that sum are particularly large (possibly indicating a bug in my use of CustomNonbondedForce).

@peastman
Copy link
Member

peastman commented Dec 3, 2017

The current head revision is 07c1b86. Let's take that as our initial candidate for the beta. Shall we try building conda packages based on it?

@jchodera
Copy link
Member Author

jchodera commented Dec 4, 2017

See omnia-md/conda-recipes#837

@jchodera
Copy link
Member Author

jchodera commented Dec 4, 2017

OK, we're starting to push the beta to anaconda cloud, but what I feared would happen is happening: Only one version number can be in a package channel at a time, regardless of label. The beta label is replacing the same package versions with the dev label and will be wiped out tonight by the dev builds.

Last time, I realize we built and pushed to the openmm-beta package channel. I'll rejigger things to do that now and re-push the dev labels.

@jchodera
Copy link
Member Author

jchodera commented Dec 4, 2017

The beta 7.2.0 and dev 7.2.1 builds are pushed for linux, but osx is still backlogged:
https://anaconda.org/omnia/openmm/files

@peastman
Copy link
Member

peastman commented Dec 5, 2017

Because building Windows packages can never work correctly the first time, even if I left everything in a working state...

conda.exceptions.UnsatisfiableError: The following specifications were found to be in conflict:
  - sphinx ==1.5.6
  - sphinxcontrib-autodoc_doxygen -> sphinx ==1.3.1

Why is this error happening on Windows but not the other platforms? Do we need @rmcgibbo to build a new package of sphinxcontrib-autodoc_doxygen?

@peastman
Copy link
Member

peastman commented Dec 5, 2017

For the moment, I'm working around it by changing meta.yaml to not specify a particular version of Sphinx. Strangely, that causes it to pick 1.5.1. Shouldn't that still conflict with sphinxcontrib-autodoc_doxygen? But somehow it works, so I don't understand what's going on.

@peastman
Copy link
Member

peastman commented Dec 5, 2017

Windows builds are done. I still need to test them, but once that's done, shall I go ahead and post the announcement?

@jchodera
Copy link
Member Author

jchodera commented Dec 5, 2017

Sure! Do you want to draft a preview of the feature and bugfix list for the release?

@jchodera
Copy link
Member Author

jchodera commented Dec 5, 2017

Did we already update the User Guide with the new forcefields and recommendations?

Should we also add information about how the openmm-forcefields conda package will contain additional CHARMM and Amber forcefields available in charmm/... and amber/... paths? Or would you rather we just document all of that in the README at http://github/choderalab/openmm-forcefields?

Please do note in the release notes / announcement that we are still testing the converted forcefields for compatibility (with each other) and energy fidelity to the original programs (CHARMM and Amber). I'm not able to finish that up until the weekend.

@jchodera
Copy link
Member Author

jchodera commented Dec 5, 2017

We should also probably include information on the provenance of the files you included in the OpenMM distribution:

  • Where the conversion scripts live
  • Who contributed to those scripts and their validation (as well as huge acknowledgements to Jason Swails, David Case, and Alex MacKerell for their help)
  • How they were tested
  • What source parameter sets (AmberTools17 and the MacKerell CHARMM36 distribution) were used

@peastman
Copy link
Member

peastman commented Dec 5, 2017

Announcement is posted! https://simtk.org/plugins/phpBB/viewtopicPhpbb.php?f=161&t=8493&p=0&start=0&view=&sid=4688c90feb0b0cc33e666749f37f7c5f

Did we already update the User Guide with the new forcefields and recommendations?

Yes.

Should we also add information about how the openmm-forcefields conda package will contain additional CHARMM and Amber forcefields

Sounds like that would be worth mentioning in the manual.

@peastman
Copy link
Member

peastman commented Jan 8, 2018

I haven't heard of any problems with the beta, and #1967 is the last change I have planned. Once that's merged, are we ready to build the release candidate?

@peastman
Copy link
Member

peastman commented Jan 8, 2018

#1967 is merged. The current revision is 7164109. Is everyone ok with building the release candidate based on it?

@peastman
Copy link
Member

@jchodera? @Lnaden? Anyone?

@jchodera
Copy link
Member Author

Sorry for the delay---I'm stuck writing a grant proposal until Monday, after which I'm hoping to finish all the energy validation tests.

Has anybody reported successful heavy use of much of the new forcefields? Have you been able to, for example, push a lot of proteins through pdbfixer or the new membrane builder using the new forcefields?

@peastman
Copy link
Member

I've received very little feedback from anyone.

@jchodera
Copy link
Member Author

The holidays were probably part of the reason things have been slow. We can put together a release candidate and advertise a bit more to get people to test it more thoroughly, but I imagine we have to find a way to just push a lot more systems through in an automated manner to see if things break.

1 similar comment
@jchodera
Copy link
Member Author

The holidays were probably part of the reason things have been slow. We can put together a release candidate and advertise a bit more to get people to test it more thoroughly, but I imagine we have to find a way to just push a lot more systems through in an automated manner to see if things break.

@peastman
Copy link
Member

Unless someone runs into problems, I rarely hear much from beta testers. I suspect most people don't do much more than install it, run their existing files, and make sure it doesn't crash. We can't expect any sort of thorough testing from them. That's our job.

What's the status of the force field validation? A month and a half ago you said you were working on it (#1611 (comment)), so I assumed by now it was long since finished.

@jchodera
Copy link
Member Author

It's not finished yet---we had a ton of papers to get submitted by the end of the year, and the lab shuts down mid-Dec through mid-Jan. I still need to update the CHARMM validation scripts from OpenMM 7.1 to test 7.2, and then create some more Amber integration tests that feature more complex protein and solvent systems, but I won't be able to do any of that until next week. If you want to take a stab at either of these before then, I can give you details on what needs to be done.

I'm also still waiting for ParmEd to cut a new release before I can formally release the openmm-forcefield package. All of the PRs needed for that have been merged as of a few days ago, fortunately.

The good news is that we are going to be able to hire some more software developers/postdocs that can help on the OpenMM front soon.

@peastman
Copy link
Member

That sounds like you're saying it's going to be at least month?

@jchodera
Copy link
Member Author

No, it shouldn't take long. I think we just need a dedicated day each for testing CHARMM and AMBER more exhaustively, and we can do this in parallel with the release candidate testing. I've just pinged the ParmEd channel about the release. I can set aside The and Thu of next week to finish this up on my end.

@peastman
Copy link
Member

Let's go ahead and build the release candidate then. The plan will be that if no problems are found, it will become the official release on Friday of next week.

@jchodera
Copy link
Member Author

Sounds great. BTW, I'll be at Stanford Thu afternoon---I'll stop by and say hi!

@peastman
Copy link
Member

It looks like all the builds finally managed to go through. I just posted an announcement on the forum.

@peastman
Copy link
Member

@jchodera a user on the forum found a serious bug in the CHARMM36 implementation:

https://simtk.org/plugins/phpBB/viewtopicPhpbb.php?f=161&t=8588&p=23617&start=0&view=&sid=8c730afb4f775db12b7be6ddd6304313

All the water models list an extra bond between the two hydrogens. For example,

<Residue name="TIP3">
  <Atom charge="-0.834" name="OH2" type="OT"/>
  <Atom charge="0.417" name="H1" type="HT"/>
  <Atom charge="0.417" name="H2" type="HT"/>
  <Bond atomName1="OH2" atomName2="H1"/>
  <Bond atomName1="OH2" atomName2="H2"/>
  <Bond atomName1="H1" atomName2="H2"/>
</Residue>

The result is that if you try to use it for any system involving water, it doesn't find a matching template.

@jchodera
Copy link
Member Author

jchodera commented Jan 20, 2018

That's actually how they're specified in CHARMM:

RESI TIP3         0.000 ! tip3p water model, generate using noangle nodihedral
GROUP
ATOM OH2  OT     -0.834
ATOM H1   HT      0.417
ATOM H2   HT      0.417
BOND OH2 H1 OH2 H2 H1 H2    ! the last bond is needed for shake

@peastman
Copy link
Member

But it's incorrect for an OpenMM force field. If the template lists a bond there, it won't match water molecules.

@jchodera
Copy link
Member Author

I guess that makes it clear why those integration tests are so important! :)

This raises some important questions:

  • Should we be automatically editing all water models to depart from what CHARMM specifies and remove that bond, especially if there may not be parameters to treat the flexible version of this model?
  • The water models in CHARMM are named things like TIP3, TP3M, TIP4, etc., but OpenMM hard-code water residue names. If we remove the H1-H1 bond, ow can we generalize the rigidWater code in ForceField to actually detect water models?

@jchodera
Copy link
Member Author

An alternative would be to have an option that adds H1-H1 bonds to the topology prior to calling ForceField.createSystem().

@peastman
Copy link
Member

TIP3, TP3M, etc. are the names of templates. It doesn't care what those are called. HOH is the name of the residue in the topology. Those are required to follow the standard PDB naming convention, which means water is always called HOH.

Should we be automatically editing all water models to depart from what CHARMM specifies

We should be accurately translating the CHARMM definitions into correct OpenMM definitions that give identical energies. Recording a bond between the two hydrogens is not a correct OpenMM definition of a water template. On the other hand, if a water model is supposed to include a harmonic Urey-Bradley term there, we need to make sure the force field includes that too. I don't believe any of them actually do that, but we need to verify to make sure.

@jchodera
Copy link
Member Author

I've manually checked all the CHARMM water model definitions to make sure there are no Urey-Bradley terms---I didn't find any.

I'll add the capability to our CHARMM conversion script to direct that H-H bonds be removed from the residue templates. I'll add a system containing protein, waters, and ions to test.

@peastman
Copy link
Member

Great, thanks!

@jchodera
Copy link
Member Author

There's a bit of ambiguity in terms of how to patch ParmEd to handle this, so I've asked @swails here: ParmEd/ParmEd#941

In the meantime, I've got a few test systems prepared in CHARMM-GUI for integration tests:

  • 1VII - 36-residue villin headpiece with waters and ions
  • 7DFR - DHFR with waters and ions
  • 1UW1 - small protein with ADP, Zn(II), waters, ions
  • 1XRD - transmembrane helix with lipids, sterols, waters, ions

@jchodera
Copy link
Member Author

@peastman : I've run into one more previously-unnoticed issue with the water model conversions: When planning the original conversion, we weren't examining the contents of the non_charmm/ directory, which we later added the additional water models from. As a result, we overlooked adding handling for the LONEPAIR card needed to support multisite water models.

I'll get the TIP3P conversion fixed first, then focus on updating the water models that require lone pairs.

@peastman
Copy link
Member

The new release candidate is now online.

@peastman
Copy link
Member

peastman commented Feb 6, 2018

Is all the force field testing done now? It's been almost a week since rc2 was posted, so we should be getting ready to release it.

@jchodera
Copy link
Member Author

jchodera commented Feb 6, 2018

I'll need a few more days, but we should be good to go on Monday.

@peastman
Copy link
Member

peastman commented Feb 6, 2018

In that case I'm going to start merging post 7.2 features. I have a growing backlog of changes waiting to be merged, and it's getting inconvenient. If we need to make any more changes to 7.2, we can create a branch for them.

@jchodera
Copy link
Member Author

jchodera commented Feb 6, 2018

Sounds good.

@jchodera
Copy link
Member Author

@peastman: I've implemented a more direct energy comparison directly to the free academic version of CHARMM. I cannot get the energies to agree. See openmm/openmmforcefields#61

I suggest we delay final release until we are able to track down the origin of these energy discrepancies. Any help would be appreciated.

@peastman
Copy link
Member

It's been a week since rc3 was posted. Shall I go ahead and officially release it?

@jchodera
Copy link
Member Author

Sure. I'm focusing on getting all the ParmEd PRs accepted and releases cut for openmm-forcefields so we can at least be sure the parameters can be converted reproducibly.

@peastman
Copy link
Member

And it's released!

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

No branches or pull requests

4 participants