-
Notifications
You must be signed in to change notification settings - Fork 86
Make default SMIRNOFF force field openff-2.2.1
#417
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
Conversation
mattwthompson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I did some aggressive searching elsewhere in the repo, please consider each of these suggestions optional (they're mostly out of scope, too, and could be split out into separate issues & PRs if I'm not mistaken on their value):
- A changelog entry, if we're doing that during development (at release time is also fine)
- A look at
template_generator_kwargsin the Espaloma code, which says 2.0.0 is used by default. From a glance through the code, I think the documentation is correct, but it could also be updated to 2.2.1. - The
_filter_openfftesting utility hard-codes a solution around"openff-2.0.0-rc.1". OpenFF has recently introduced more pre-release force fields, two RCs of 2.3.0 and an alpha of 3.0.0. I would expect this to cause failing tests if the most recentopenff-forcefieldsis installed and all available force fields are iterated through
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #417 +/- ##
=======================================
Coverage 84.17% 84.17%
=======================================
Files 5 5
Lines 771 771
=======================================
Hits 649 649
Misses 122 122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think we usually update this at release time.
I saw this too; should have asked about it. I didn't update it since it didn't look to be updated last time to 2.1.0 and I wasn't sure if there was an Espaloma-specific reason for this. Do you know what the "unconstrained" force fields are, and if there's a reason the Espaloma generator uses one (
This shouldn't have to do with them being prereleases; it was because 2.0.0rc1 has a bad value (see #330). I see at least one test is installing the |
They're the same but without a parameter that matches all bonds between hydrogens and heavy atoms. It should be functionally analogous to
My guess is it has something to do with Interchange's behavior around constrained bonds; OpenMM doesn't need to know the harmonic bond parameters if the bond is going to be constrained, but Espaloma might. The paper trail on Interchange's behavior starts here and may or may not have changed since the decision to use the unconstrained one with Espaloma. In any case, we should leave it place with Espaloma. I'm not sure why the unconstrained ones aren't used in the SMIRNOFF template generator. I would have guessed the opposite, since bond information can easily be discarded but can't be written back in if it's not there. I could be wrong (and certainly don't think we should change this now).
Thanks for jogging my memory! I had forgotten about the specific issue with that force field. |
|
OK, I think we can make |
|
Tests I'd assume should be unrelated are failing. They're GAFF tests with the molecule I'm going to run the test again to see if the failure is reproducible on CI. |
|
The test that was failing before doesn't appear to have failed again. I can't reproduce this locally and it shouldn't be affected by the changes here. We should keep an eye out for it though. GitHub still won't let me merge this, I think because OpenMM 8.2 was still being tested when the PR was opened? |
Resolves #409.