-
Notifications
You must be signed in to change notification settings - Fork 35
add espaloma tests #1450
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
add espaloma tests #1450
Conversation
To disambiguate between esaplaoma and espaloma charge, we need to track HAS_ESPALOMA separately from HAS_ESPALOMA_CHARGE
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1450 +/- ##
==========================================
- Coverage 95.25% 93.21% -2.05%
==========================================
Files 172 172
Lines 14487 14498 +11
==========================================
- Hits 13800 13514 -286
- Misses 687 984 +297
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:
|
|
We will need to update the logic when we start testing on python 3.14 |
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.
just one question
| run: echo "date=$(date +%Y-%m-%d)" >> "${GITHUB_OUTPUT}" | ||
|
|
||
| - name: "Setup Micromamba" | ||
| if: ${{ matrix.python-version != '3.13' }} |
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.
do you expect this to work with python 3.14, or should this be <'3.13'?
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.
idk how to do a less than for a string comparison, it really depends what happens in conda-forge land for rebuilds for different python versions.
|
|
||
|
|
||
| @pytest.mark.skipif(not HAS_ESPALOMA, reason='espaloma is not available') | ||
| def test_dry_run_espaloma_vacuum(benzene_vacuum_system, tmpdir): |
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.
Ideally, we should check more things, like if the partial charges made it through, etc.. I'm ok with this as a smoke test, but we should raise a separate issue to try it out in a production setting.
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.
Agreed, I'll raise an issue, maybe we can add a GPU test where we run some MD with parameters from espaloma.
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
|
No API break detected ✅ |
Checklist
newsentryDevelopers certificate of origin
Fixes #1449