-
Notifications
You must be signed in to change notification settings - Fork 35
Import vendored openff.models from gufe #1260
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1260 +/- ##
==========================================
- Coverage 94.66% 92.63% -2.03%
==========================================
Files 143 143
Lines 10957 10957
==========================================
- Hits 10372 10150 -222
- Misses 585 807 +222
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:
|
|
This is needed to support numpy2/newer python versions since openff.models is no longer maintained and the pins on the conda-forge package will keep our ecosystem back. |
|
I also just bump the python version to sync with google colab |
atravitz
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.
looks good! could you add a news item, if this is technically the commit that makes us numpy 2 compatible?
Eh, like our code has been numpy 2 compatible since the day it was written so I am always a bit weary about saying that is what this actually does... I think it is worth mentioning in a news bit that we now import a vendored copy of openff.models from gufe, it is on the edge of being kinda internal but I could see someone going down a rabbit hole on how they installed openff-models but things are being weird, so the change log would be a good hint for that |
I somewhat disagree, we shouldn't have a news item for this, or at least right now I don't see the value it brings to users. I do however agree with Alyssa that we should just put a news item that says "makes toolkit compatible with numpy 2.0". The news items aren't meant to be pure accurate representation of reality but rather a summary of "what am I getting out of this" for users. My compromise here is that you just don't add a PR number and leave it as just an entry in the news. |
I can live with that! |
|
No API break detected ✅ |
Checklist
newsentryDevelopers certificate of origin