ENH: support tpx 133 (GMX 2024.1)#4523
Conversation
* 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.
|
Hello @tylerjereddy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-03-25 22:24:16 UTC |
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.
Please have a look at the Please note: The |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
|
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. |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
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?
I pushed in what I could, but as I noted above, I don't know how to find out the |
|
@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) |
|
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? |
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
tylerjereddy
force-pushed
the
treddy_tpx_133
branch
from
f43b047 to
26098bb
Compare
| 122 28 2021 yes | ||
| 127 28 2022 yes | ||
| 129 28 2023 yes | ||
| 133 28 2024.1 yes |
There was a problem hiding this comment.
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>
* 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
* ENH: support tpx 134 (GMX 2024.4) * Fixes gh-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 gh-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.
* 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.
4 participants
One of my postdocs (Emvia) needs parsing of newer
.tprfiles to work; we actually are using tpx version130internally, but that wasn't from a stable release, so I just went for2024.1and presumbly we can just useconvert-tprto "up-version" to the stable release version internally.This patch used
convert-tprwith GMX2024.1to mimic Jonathan's changes from Support TPR files produced by gromacs 2023 #4052, when we last bumpedtpxversion. 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
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4523.org.readthedocs.build/en/4523/