Skip to content

Conversation

@tylerjereddy
Copy link
Member

@tylerjereddy tylerjereddy commented Dec 26, 2024

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


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

* Fixes MDAnalysisgh-4855.

* Add support for reading topology data from `.tpr` files
produced by GROMACS 2024.4, corresponding to tpx version
`134`.

* My approach was similar to the one I used in MDAnalysisgh-4523, except
for two things: 1) I retrieved the current generation (`28`) from
a `print` in our own binary parser rather than rebuilding
GMX from source with added `printf`, and more importantly 2) I
had to add some shims because the `.tpr` format has changed.

* With regard to the additional field data now stored in the
`.tpr` format, I was involved in reviewing it upstream at:
https://gitlab.com/gromacs/gromacs/-/merge_requests/4544.
You can see that the changes related to the MARTINI functional
form made it into the `v2024.4` tag i.e., here:
https://gitlab.com/gromacs/gromacs/-/blob/v2024.4/src/gromacs/gmxpreprocess/convparm.cpp?ref_type=tags#L372
@pep8speaks
Copy link

pep8speaks commented Dec 26, 2024

Hello @tylerjereddy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 435:80: E501 line too long (94 > 79 characters)

Comment last updated at 2024-12-27 17:25:12 UTC

@codecov
Copy link

codecov bot commented Dec 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.63%. Comparing base (c08cb79) to head (daa16ce).
Report is 36 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4866      +/-   ##
===========================================
- Coverage    93.65%   93.63%   -0.03%     
===========================================
  Files          177      189      +12     
  Lines        21779    22853    +1074     
  Branches      3064     3067       +3     
===========================================
+ Hits         20398    21399    +1001     
- Misses         929     1002      +73     
  Partials       452      452              

☔ 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

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

Tiny typo but thanks for doing this!

Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

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

LGTM besides the comment from @hmacdope.

Copy link
Contributor

@tanishy7777 tanishy7777 Dec 27, 2024

Choose a reason for hiding this comment

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

I was going through this PR and I am not able to understand why u added an elif for fver>=134. I am a bit new to this repo.

* Remove the periods (`.`) from Tyler's identifier
in the `CHANGELOG` to satisfy reviewer comments.
@hmacdope hmacdope merged commit 5656fe4 into MDAnalysis:develop Dec 28, 2024
22 of 24 checks passed
@tylerjereddy tylerjereddy deleted the treddy_issue_4855_tpx_134 branch December 28, 2024 02:55
tylerjereddy added a commit to tylerjereddy/mdanalysis that referenced this pull request 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 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.
amruthesht pushed a commit to amruthesht/mdanalysis that referenced this pull request Feb 19, 2025
* ENH: support tpx 134 (GMX 2024.4)

* Fixes MDAnalysisgh-4855.

* Add support for reading topology data from `.tpr` files
produced by GROMACS 2024.4, corresponding to tpx version
`134`.

* My approach was similar to the one I used in MDAnalysisgh-4523, except
for two things: 1) I retrieved the current generation (`28`) from
a `print` in our own binary parser rather than rebuilding
GMX from source with added `printf`, and more importantly 2) I
had to add some shims because the `.tpr` format has changed.

* With regard to the additional field data now stored in the
`.tpr` format, I was involved in reviewing it upstream at:
https://gitlab.com/gromacs/gromacs/-/merge_requests/4544.
You can see that the changes related to the MARTINI functional
form made it into the `v2024.4` tag i.e., here:
https://gitlab.com/gromacs/gromacs/-/blob/v2024.4/src/gromacs/gmxpreprocess/convparm.cpp?ref_type=tags#L372

* DOC: PR 4866 revisions

* Remove the periods (`.`) from Tyler's identifier
in the `CHANGELOG` to satisfy reviewer comments.
tylerjereddy added a commit to tylerjereddy/mdanalysis that referenced this pull request Mar 10, 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 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.
IAlibay pushed a commit that referenced this pull request Mar 10, 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 gh-4866.

* Some other `.tpr` additions are noted at #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>
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.

Error: Your tpx version is 134, which this parser does not support, yet

5 participants