Skip to content

Conversation

@tylerjereddy
Copy link
Member

@tylerjereddy tylerjereddy commented Feb 18, 2025

  • Add support for GROMACS 2025.0 .tpr version 137, which mostly involves accounting for the new PyTorch neural network potential field, F_ENNPOT. Most of the other changes are similar to the approach I previously used for bumping .tpr version support in ENH: support tpx 134 (GMX 2024.4) #4866.

  • Some other .tpr additions are noted at GROMACS 2025 TPR support #4888, but we don't appear to need to use those within the context of our current .tpr parsing tests. It may be sensible to occasionally generate .tpr test files that use those extra bells and whistles to see how we fare, though as a first pass here the full testsuite passes for us if we simply do the usual gmx convert-tpr at the latest stable GROMACS release tag v2025.0 and propagate our current TPR testing to additionally use the converted files.

PR Checklist

  • Issue raised/referenced?
  • Tests updated/added?
  • Documentation updated/added?
  • package/CHANGELOG file updated?
  • Is your name in package/AUTHORS? (If it is not, add it!)

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.


📚 Documentation preview 📚: https://mdanalysis--4919.org.readthedocs.build/en/4919/

elif i in [setting.F_ENNPOT]:
# TODO: handle the new neural network potentials
# supported from GROMACS 2025.0
pass
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this and other new v2025.0 bells and whistles, cc @al42and for possible generation of new test files that have the new bells and whistles.

I don't now if we really need a new test file with i.e., the new PyTorch potential handling + reference coordinate scaling with multiple centers of mass + init-histogram-counts to enable continuation of expanded ensemble runs.

Probably would be good to have eventually, if only to make sure we don't need to skip over any other bits of information when "seeking" through the file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we think there are attributes we will be missing if we're not properly parsing the NN potential things? i.e. what I'm asking is if we need a warning here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this and other new v2025.0 bells and whistles, cc @al42and for possible generation of new test files that have the new bells and whistles.

I can make a TPR with a bunch of things turned on, but there are issues if we want to stick with existing test sets:

  • 2lyz: they only have TPR, which is neither editable nor "decompilable" into parts; can't change settings there (and looks like the original receipt is lost in time, since the new are just converted from the old ones, per 336ff1d).
  • bonded/dummy: currently, for NN, we need something with "physical" element names.

If that's ok, I can create a new TPR roughly based on GROMACS's own tests.
Reconstructing 2lyz and making a test based on it is doable, but I'd rather avoid that as long as you're ok with adding a new test system.

@codecov
Copy link

codecov bot commented Feb 18, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.61%. Comparing base (596cd7c) to head (cd912fd).
Report is 25 commits behind head on develop.

Files with missing lines Patch % Lines
package/MDAnalysis/topology/tpr/utils.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4919      +/-   ##
===========================================
- Coverage    93.64%   93.61%   -0.04%     
===========================================
  Files          177      189      +12     
  Lines        21857    22925    +1068     
  Branches      3077     3078       +1     
===========================================
+ Hits         20469    21461     +992     
- Misses         938     1013      +75     
- Partials       450      451       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very versed in the new 2025 changes, but this looks reasonable to me, thanks!

@tylerjereddy
Copy link
Member Author

Reconstructing 2lyz and making a test based on it is doable, but I'd rather avoid that as long as you're ok with adding a new test system.

@al42and I think making a new .tpr unrelated to our current testing .tprs would still be useful if it has the new bells and whistles turned on, especially if you could keep the file size suitable for a testsuite (i.e., similar file size range to what we currently use). Presumably we could at least use gmx dump to get some ground truths for sanity checking our parser.

@IAlibay IAlibay mentioned this pull request Mar 1, 2025
@al42and
Copy link
Contributor

al42and commented Mar 2, 2025

Hi!

Here's an example file + test. The NN potential is stored as a reference, so there's not really much to test except that it does not screw up anything else. Also enabled a few other random features in the MDP. The file size is 11KiB (that's just ALA dipeptide in vacuum)

e6e8bb5

@IAlibay IAlibay added this to the Release 2.9.0 milestone Mar 9, 2025
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the capability to read the basic features for the new format (as demonstrated in the PR on the converted TPR files) is "good enough" for getting this into the 2.9.0 release #4939 . Given the time constraints I'd suggest to follow up with further work in additional PRs.

If we're worried that other things really won't work well then we could add a note to docs/release notes. @tylerjereddy I'll leave this to your judgement.

tylerjereddy and others added 2 commits March 9, 2025 21:48
* Add support for GROMACS `2025.0` `.tpr` version 137, which mostly
involves accounting for the new PyTorch neural network potential
field, `F_ENNPOT`. Most of the other changes are similar to the
approach I previously used for bumping `.tpr` version support
in MDAnalysisgh-4866.

* Some other `.tpr` additions are noted at MDAnalysis#4888,
but we don't appear to need to use those within the context of our
current `.tpr` parsing tests. It may be sensible to occasionally
generate `.tpr` test files that use those extra bells and whistles to
see how we fare, though as a first pass here the full testsuite passes
for us if we simply do the usual `gmx convert-tpr` at the latest stable
GROMACS release tag `v2025.0` and propagate our current TPR testing
to additionally use the converted files.
* Generated input file with random set of features enabled
Using inputs from gromacs/src/testutils/simulationdatabase/
`gmx grompp -f tt6710474.mdp -c  ala.gro -n ala.ndx -p ala.top  -o ala_nnpot_gmx_2025_0.tpr`

* Add Andrey to `CHANGELOG`
@tylerjereddy tylerjereddy force-pushed the treddy_support_gmx_2025.0 branch from 2504fd8 to 2522224 Compare March 10, 2025 02:49
@tylerjereddy
Copy link
Member Author

I'm at the airport in Austin, but just pulled in Andrey's commit and the testsuite seems to be happy locally even with the fancy new TPR, so I went ahead and pushed that up alongside Andrey's addition to the CHANGELOG.

So, no known reservations on my end, may even be able to remove the residual TODO, though that doesn't hurt

@IAlibay
Copy link
Member

IAlibay commented Mar 10, 2025

Amazing, thanks @al42and and @tylerjereddy !

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

black linting (couldn't push from the CLI for some reason)

@IAlibay
Copy link
Member

IAlibay commented Mar 10, 2025

CI was passing earlier and now that linters are I'm going to go ahead with the merge.

@IAlibay IAlibay merged commit c83d030 into MDAnalysis:develop Mar 10, 2025
14 of 22 checks passed
@tylerjereddy tylerjereddy deleted the treddy_support_gmx_2025.0 branch March 10, 2025 15:48
Abdulrahman-PROG pushed a commit to Abdulrahman-PROG/mdanalysis that referenced this pull request Apr 13, 2025
* ENH: tpx 137 support

* Add support for GROMACS `2025.0` `.tpr` version 137, which mostly
involves accounting for the new PyTorch neural network potential
field, `F_ENNPOT`. Most of the other changes are similar to the
approach I previously used for bumping `.tpr` version support
in MDAnalysisgh-4866.

* Some other `.tpr` additions are noted at MDAnalysis#4888,
but we don't appear to need to use those within the context of our
current `.tpr` parsing tests. It may be sensible to occasionally
generate `.tpr` test files that use those extra bells and whistles to
see how we fare, though as a first pass here the full testsuite passes
for us if we simply do the usual `gmx convert-tpr` at the latest stable
GROMACS release tag `v2025.0` and propagate our current TPR testing
to additionally use the converted files.

* TST: Add GROMACS 2025.0 TPR with NNpot

* Generated input file with random set of features enabled
Using inputs from gromacs/src/testutils/simulationdatabase/
`gmx grompp -f tt6710474.mdp -c  ala.gro -n ala.ndx -p ala.top  -o ala_nnpot_gmx_2025_0.tpr`

* Add Andrey to `CHANGELOG`

* Apply suggestions from code review

---------

Co-authored-by: Andrey Alekseenko <al42and@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants