Skip to content
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

ENH: support tpx 133 (GMX 2024.1) #4523

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

tylerjereddy
Copy link
Member

@tylerjereddy tylerjereddy commented Mar 21, 2024

  • One of my postdocs (Emvia) needs parsing of newer .tpr files to work; we actually are using tpx version 130 internally, but that wasn't from a stable release, so I just went for 2024.1 and presumbly we can just use convert-tpr to "up-version" to the stable release version internally.

  • This patch used convert-tpr with GMX 2024.1 to mimic Jonathan's changes from Support TPR files produced by gromacs 2023 #4052, when we last bumped tpx version. The full testsuite passed for me locally at least, so maybe we'll be lucky enough to have another simple bump.

  • Note that I did not update the documentation or the CHANGELOG, fell free to push that in here if you're confident of the various tpx generation things, etc.

PR Checklist

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

Developers certificate of origin


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

* One of my postdocs (Emvia) needs parsing of newer
`.tpr` files to work; we actually are using tpx version
`130` internally, but that wasn't from a stable release,
so I just went for `2024.1` and presumbly we can just use
`convert-tpr` to "up-version" to the stable release version.

* This patch used `convert-tpr` with GMX `2024.1` to mimic
Jonathan's changes from MDAnalysisgh-4052, when we last bumped `tpx`
version. The full testsuite passed for me locally at least, so
maybe we'll be lucky enough to have another simple bump.

* Note that I did not update the documentation or the `CHANGELOG`,
fell free to push that in here if you're confident of the various
tpx generation things, etc.
@pep8speaks
Copy link

pep8speaks commented Mar 21, 2024

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

Line 41:80: E501 line too long (83 > 79 characters)

Line 423:80: E501 line too long (90 > 79 characters)

Comment last updated at 2024-03-25 22:24:16 UTC

Copy link

github-actions bot commented Mar 21, 2024

Linter Bot Results:

Hi @tylerjereddy! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/8427796799/job/23079042250


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.63%. Comparing base (0582265) to head (26098bb).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4523      +/-   ##
===========================================
- Coverage    93.65%   93.63%   -0.03%     
===========================================
  Files          168      180      +12     
  Lines        21215    22294    +1079     
  Branches      3908     3908              
===========================================
+ Hits         19869    20875    +1006     
- Misses         888      961      +73     
  Partials       458      458              

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

@IAlibay IAlibay self-requested a review March 22, 2024 12:42
@IAlibay
Copy link
Member

IAlibay commented Mar 22, 2024

Thanks for working on this @tylerjereddy - I was also looking at this a few weeks back (got as far as John telling me to use convert-tpr 😅) and got distracted, I'll try to give it a review as soon as possible.

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.

If the test files are TPR 133 and all tests pass then that looks good enough for me.

We'd just need the docs + CHANGELOG. Could you quickly add those in, please?

@orbeckst orbeckst self-assigned this Mar 25, 2024
@tylerjereddy
Copy link
Member Author

tylerjereddy commented Mar 25, 2024

We'd just need the docs + CHANGELOG. Could you quickly add those in, please?

@orbeckst

I pushed in what I could, but as I noted above, I don't know how to find out the tpx generation of a .tpr file (not the same as the tpr version). Comically, when I asked Google Gemini, it told me to use MDAnalysis to do it, presumably because it had slurped up this exact docs table. I just put in ?? for now--IMO it is the .tpr version proper that really means something to folks anyway.

@IAlibay
Copy link
Member

IAlibay commented Mar 25, 2024

@tylerjereddy if I remember correctly this is the enum class that gets used for the generation: https://gitlab.com/gromacs/gromacs/-/blob/main/src/gromacs/fileio/tpxio.cpp?ref_type=heads#L190

Should still be 28 I think? (sorry it's late and I'm not in a C++ frame of mind)

@IAlibay
Copy link
Member

IAlibay commented Mar 25, 2024

Just briefly putting this here because it's something @jbarnoud mentioned in a DM but I didn't get a chance to dig into it - apparently there are (edit: ITP?) input files for "all bonded" and "virtual sites" TPR tests somewhere that might need adding too?

@tylerjereddy
Copy link
Member Author

apparently there are input files for "all bonded" and "virtual sites" TPR tests somewhere that might need adding too?

pretty sure I did that in the diff already, I'll see if I can dig up the obscure generation thing

@IAlibay
Copy link
Member

IAlibay commented Mar 25, 2024

apparently there are input files for "all bonded" and "virtual sites" TPR tests somewhere that might need adding too?

pretty sure I did that in the diff already, I'll see if I can dig up the obscure generation thing

🤦🏾‍♂️ Sorry I'm not paying attention - I saw the dummy one but somehow my brain just ignored it completely

* add `CHANGELOG` and TPR docs updates
@@ -66,6 +66,7 @@
122 28 2021 yes
127 28 2022 yes
129 28 2023 yes
133 28 2024.1 yes
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I added the generation as 28 based on a fresh compilation of GROMACS 2024.1 with this adjustment:

--- a/src/gromacs/fileio/tpxio.cpp
+++ b/src/gromacs/fileio/tpxio.cpp
@@ -1034,6 +1034,7 @@ static void do_inputrec(gmx::ISerializer* serializer, t_inputrec* ir, int file_v
 
     ir->tpxFileVersion = file_version;
 
+    fprintf(stderr, "****** Note: tpx_generation %d\n", tpx_generation);
     if (file_version != tpx_version)
     {
         /* Give a warning about features that are not accessible */

Then used one of the .tpr files here as follows:

gmx mdrun -s dummy_2024.tpr

which includes in its output:

<snip>
Reading file dummy_2024.tpr, VERSION 2024.1-spack (single precision)
****** Note: tpx_generation 28
****** Note: tpx_generation 28
<snip>

@orbeckst orbeckst enabled auto-merge (squash) March 25, 2024 23:10
@orbeckst orbeckst merged commit f5335b4 into MDAnalysis:develop Mar 25, 2024
21 of 23 checks passed
@tylerjereddy tylerjereddy deleted the treddy_tpx_133 branch March 25, 2024 23:33
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