Add a reader for TNG files using pytng

[hmacdope]

Fixes #3237 and partially addresses #865 (reading TNG)

This PR adds functionality for reading TNG files using pytng (finally). :smile:

The limitations of reading a TNG file using pytng are also clearly outlined in the docs.

Changes made in this Pull Request:

• Add a reader for TNG files using PyTNG

TODO

Need to add tests using an example data file

PR Checklist

• [x] Tests?
• [X] Docs?
• [x] CHANGELOG updated?
• [X] Issue raised/referenced?

[pep8speaks]

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

• In the file package/MDAnalysis/coordinates/TNG.py:

Line 29:80: E501 line too long (98 > 79 characters) Line 195:80: E501 line too long (85 > 79 characters) Line 201:80: E501 line too long (86 > 79 characters) Line 207:80: E501 line too long (82 > 79 characters) Line 261:80: E501 line too long (85 > 79 characters) Line 467:80: E501 line too long (82 > 79 characters)

• In the file testsuite/MDAnalysisTests/coordinates/test_tng.py:

Line 136:80: E501 line too long (87 > 79 characters) Line 140:80: E501 line too long (80 > 79 characters) Line 144:80: E501 line too long (80 > 79 characters)

• In the file testsuite/MDAnalysisTests/datafiles.py:

Line 130:80: E501 line too long (96 > 79 characters) Line 132:80: E501 line too long (95 > 79 characters) Line 339:80: E501 line too long (92 > 79 characters) Line 340:80: E501 line too long (95 > 79 characters)

Comment last updated at 2022-09-09 18:03:04 UTC

[orbeckst]

Does pytng have conda packages already?

[hmacdope]

Does pytng have conda packages already?

Yep ! https://github.com/conda-forge/pytng-feedstock

[codecov[bot]]

Codecov Report

Base: 94.41% // Head: 94.44% // Increases project coverage by +0.03% :tada:

Coverage data is based on head (a56022c) compared to base (8886efa). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3765      +/-   ##
===========================================
+ Coverage    94.41%   94.44%   +0.03%     
===========================================
  Files          194      195       +1     
  Lines        25249    25423     +174     
  Branches      3493     3526      +33     
===========================================
+ Hits         23838    24012     +174     
  Misses        1367     1367              
  Partials        44       44              

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/TNG.py	100.00% <100.00%> (ø)
package/MDAnalysis/coordinates/__init__.py	100.00% <100.00%> (ø)
MDAnalysisTests/coordinates/__init__.py	100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[hmacdope]

Looks good but would be good to add to coverage. In particular, what would we lose from not exposing the block based properties? This would simplify the Reader.

The additional blocks that match the stride of the position/vel/forces blocks are read into ts.data. Is that what you mean? Or do you mean you would like the flexibility to read all the special_blocks at different strides. Or do you mean just making things like self._block_dict public?

Looks good but would be good to add to coverage

Proving a bit of a challenge with the optional dependency, I'll have a look at how it was done for H5MD etc.

[richardjgowers]

@hmacdope you should be able to slap a pytest.importorskip to make all the tests optional. I'd remove the properties about blocks to reduce the API surface and make it easier to maintain.

[IAlibay]

@hmacdope you should be able to slap a pytest.importorskip to make all the tests optional. I'd remove the properties about blocks to reduce the API surface and make it easier to maintain.

You'll want to not do that for everything, you want to test the error message you get if you don't have it installed

[hmacdope]

PyTNG seems to have some trouble reading files ( specifically a file made from test.xtc and/or test.trr) that have gone through trjconv as their block strides are set to 100 but the step size does not appear to be updated. This is roughly covered in this issue https://github.com/MDAnalysis/pytng/issues/95. Perhaps I will query the GMX people as well.

[hmacdope]

PyTNG seems to have some trouble reading files ( specifically a file made from test.xtc and/or test.trr) that have gone through trjconv as their block strides are set to 100 but the step size does not appear to be updated. This is roughly covered in this issue MDAnalysis/pytng#95. Perhaps I will query the GMX people as well.

This is fixed in a new PyTNG patch https://github.com/MDAnalysis/pytng/pull/96 so we are ready to roll forward here.

[hmacdope]

With the release of the new PyTNG tag (0.2.3) pn PyPI and Conda I think this is ready for review.

Main remaining question to answer is how many test more test files we want to introduce to cover all the cases. I'll look into making one or two more to cover bad velocity and force reads which are not currently possibly with the files we have, but this can be reviewed in the meantime. :)

[hmacdope]

I have finalised testing with some new test files. I looked into making a TNG file to address the only lines that don't have coverage (reading a non-particle block that doesn't share a stride with a particle block) but this doesn't appear possible to do within GROMACS. If people really want I can write a C++ program to make a file like this but it is a bit of a pain.

[orbeckst]

There's some PEP8 and resolve conflicts with develop but you can attend to any of these minor things without me having to look at everything, so it all looks good from my side.

[hmacdope]

There's some PEP8 and resolve conflicts with develop but you can attend to any of these minor things without me having to look at everything, so it all looks good from my side.

Thanks @orbeckst! Ill tidy those up ASAP.

[hmacdope]

In all the tests I wasn't sure if being serializable was also tested — is it?

Yes the tests that subclass BaseReference test serializability. :)

RE PEP8 the ones I have left in I feel are better kept on a single line for readability. Also datafiles.py is non PEP8 compliant. I wonder if we should just exclude it from the PEP8 bot all together if possible?

[orbeckst]

datafiles.py is a 🔥 🌶️ 8 mess so excluding it sounds sensible.

[orbeckst]

The new cirrus CI seems to have issues https://github.com/MDAnalysis/mdanalysis/pull/3765/checks?check_run_id=8263396553 — @tylerjereddy any insights?

[tylerjereddy]

@orbeckst I restarted manually: https://cirrus-ci.com/task/5104634413973504

I'm not so sure it will properly reflect on GitHub for this service, but seems to be running tasks again. They're probably getting hammered. Feel free to delete it or cron it or whatever if it gets too annoying.

[richardjgowers]

@IAlibay I think your review comments have been addressed here

[IAlibay]

@IAlibay I think your review comments have been addressed here

Thanks, unless this has a time sensitive component (please let me know if that is the case), I'll re-review later this week (after the 13th).

[IAlibay]

Sorry about the lack of review here, this is still on my radar the week has just been a bit busy. I'll try to go over it tomorrow.

[hmacdope]

Any chance of a final review here @MDAnalysis/coredevs? :)

[IAlibay]

Yeah sorry I did note it down earlier today when I was looking at what PRs needed merging. I'll try to re-review over the weekend.

[IAlibay]

We can ignore most of the linter stuff, but do have a look at the output if there's anything you think is worth it. Specifically the assert calls might make more sense without the brackets.

[hmacdope]

@IAlibay I think i have addressed everything including formatting. :)

[jbarnoud]

This has been years in the making. Thank you for making this milestone happen!

[orbeckst]

Well done, everyone!