Update NCDF Reader to read .ncrst Restart Files

[jeremyleung521]

Fixes #5030

Changes made in this Pull Request:

• Updated NCDF Reader to read Amber .ncrst restart files
• Added tests to verify that the ncrst file is readed correctly. The test file I used is created from a .nc file already present in MDAnalysisTest
• Noticed @jpkrowe did not add himself to the changelog, so also added him here.

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--5031.org.readthedocs.build/en/5031/

[github-actions]

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.62%. Comparing base (af9848b) to head (eb7ba03).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #5031   +/-   ##
========================================
  Coverage    93.62%   93.62%           
========================================
  Files          177      177           
  Lines        21995    22004    +9     
  Branches      3112     3115    +3     
========================================
+ Hits         20593    20602    +9     
  Misses         947      947           
  Partials       455      455           

☔ 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.

[jpkrowe]

Thanks for adding my name. I didn't notice that I hadn't added it!

[yuxuanzhuang]

Thanks! It all looks good to me.

One thing to note (and perhaps worth documenting) is that coordinates are currently always stored as float32.

[IAlibay]

Sorry I'm going to block this for further discussion. Restart files really should be treated as SingleFrameReaders and I'm not sure this is the right way to do this (given it breaks the user experience with non NC restart files).

[yuxuanzhuang]

Sorry I'm going to block this for further discussion. Restart files really should be treated as SingleFrameReaders and I'm not sure this is the right way to do this (given it breaks the user experience with non NC restart files).

That's a good point, sorry I missed that!

To avoid code duplication with NCDFReader, I assume a possible solution would be creating a mixin class to store the shared logic of nc files and have a separate NCRSTReader that inherits both the mixin class and SingleFrameReader.

Would that approach align with what you had in mind?

[IAlibay]

Sorry I'm going to block this for further discussion. Restart files really should be treated as SingleFrameReaders and I'm not sure this is the right way to do this (given it breaks the user experience with non NC restart files).

That's a good point, sorry I missed that!

To avoid code duplication with NCDFReader, I assume a possible solution would be creating a mixin class to store the shared logic of nc files and have a separate NCRSTReader that inherits both the mixin class and SingleFrameReader.

Would that approach align with what you had in mind?

I had a stalled PR a couple years back that attempted to deal with this: https://github.com/MDAnalysis/mdanalysis/pull/3942/files
I agree with your mixin idea, that was what I was missing at the time.

[jeremyleung521]

Thanks for reviewing this @IAlibay & @yuxuanzhuang! I'll try and incorporate the functionality using Mixins and the SingleFrameReader class (wasn't aware of the latter, still new to MDA development), which makes total sense to me as an approach. I'll also see if I could pull ideas (potentially code/commits) from #3942, which oddly didn't show up when I did PR/issues searches on this feature.

Sorry I'm going to block this for further discussion. Restart files really should be treated as SingleFrameReaders and I'm not sure this is the right way to do this (given it breaks the user experience with non NC restart files).

@IAlibay Was there a specific thing you're referring to with the "given it breaks the user experience with non NC restart files" comment? The current logic should be the same for using non-nc restart files (given the difference in file extensions) so I wanted to make sure I understand what you're referring to.

[IAlibay]

Was there a specific thing you're referring to with the "given it breaks the user experience with non NC restart files" comment?

What I mean here is that we have a specific single frame reader for rst7 files, which is different from the non-netcdf trj file reader. I.e. the user experience is such that we expect a different reader for the restart and trajectory file types.

[jeremyleung521]

Turning this PR into a draft for now, because there is a wide-affecting bug with the SingleFrameReaderBase's n_atoms attribute, which is causing things to fail. (#5052) Will wait for that to fix itself before moving forward, as that needs to be fixed for this to be implemented properly.