-
Notifications
You must be signed in to change notification settings - Fork 35
switch kcal/mol to kilocalorie_per_mole for roundtrip consistency #1210
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
|
@hannahbaumann you made this edit here:573f271#diff-24e303f93e84a4ff799a85a45f35209e516f389c3fa823a2f20106a89843bb25L462 so I'd like to know if there's any reason we should be keeping this as my understanding is that this is an openmm vs. openff convention difference: https://github.com/openforcefield/openff-units?tab=readme-ov-file#openmm-interoperability |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1210 +/- ##
==========================================
- Coverage 93.09% 92.57% -0.53%
==========================================
Files 141 141
Lines 10651 10667 +16
==========================================
- Hits 9916 9875 -41
- Misses 735 792 +57
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hannahbaumann
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.
Thanks @atravitz , lgtm!
ianmkenney
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.
Under the assumption that this is reasonable for the other reviewers, as they are more familiar with downstream effects, this looks good to me.
It might also be good to check to see what happens if a user provides a quantity in kcal/mol, rather than relying on the default, and if this comes up again.
That exact point was raised during our meeting so we will add a test to make sure it works |
mikemhenry
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.
Just need a test to make sure if a user sets something with 'kcal/mol' style it works -- it absolutely should since that is how this works BUT we will want that test since if we ever change the way our models work, we could introduce a regression
|
No API break detected ✅ |
mikemhenry
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!
|
Also, no need for a news entry since this really does not affect users at all |
| def instance(self): | ||
| pass | ||
|
|
||
| class TestRelativeHybridTopologyProtocolOtherUnits(GufeTokenizableTestsMixin): |
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.
I was looking at the tests earlier today and stumbled upon this - the name is really confusing. Can we maybe consider switching it out to something else? (especially having it in tokenization just threw me off).
gufe added new tests to
GufeTokenizableTestsMixinfor json and msgpack roundtripping, and it caught that roundtrippingRelativeHybridTopologyProtocolandAbsoluteSolvationProtocoldoes not reproduce the same gufe key.This is because the original object's
early_termination_target_error's units arekilocalorie_per_mole, but when it's loaded back in, it's in units ofkilocalorie / moleWe think this is because of pydantic casting, and changing the pydantic class to expect
kilocalorie_per_molesolves this.Checklist
newsentryDevelopers certificate of origin