MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.
https://mdanalysis.org
Other
1.3k stars 648 forks source link

Compatibility with Zarr Trajectory format #4557

Closed ljwoods2 closed 4 months ago

ljwoods2 commented 6 months ago

Changes made in this Pull Request:

This is just one way I am proposing to allow future compatibility with the zarrtraj format (https://github.com/Becksteinlab/zarrtraj) that I'm developing in Oliver's lab (and potentially GSOC). Since the zarrtraj format is still in the prototyping phase, there is no urgency to merge this or an alternate solution and I'm open to different ways of approaching it.

PR Checklist

Developers certificate of origin


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

github-actions[bot] commented 6 months ago

Linter Bot Results:

Hi @ljwoods2! 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 ✅ Passed

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/8531807262/job/23372008184


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

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.64%. Comparing base (73acc9b) to head (ee09673). Report is 28 commits behind head on develop.

Files with missing lines Patch % Lines
package/MDAnalysis/coordinates/chain.py 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #4557 +/- ## =========================================== - Coverage 93.66% 93.64% -0.03% =========================================== Files 168 180 +12 Lines 21248 22333 +1085 Branches 3917 3917 =========================================== + Hits 19902 20913 +1011 - Misses 888 962 +74 Partials 458 458 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hmacdope commented 5 months ago

@ljwoods2 as I think you know, this will have to be held, until the zarrtraj stuff is more mature. Is there a general case though that this PR solves? E.G chainreader cannot handle objects that are not XX or YY?

ljwoods2 commented 5 months ago

@hmacdope Yes, Oliver mentioned this issue: https://github.com/MDAnalysis/mdanalysis/issues/2884. I can come up with a better solution which in general allows object-based trajectory formats being passed in to not be caught by the ChainReader _format_hint

ljwoods2 commented 5 months ago

Maybe it's as simple as iterating through all _format_hint() methods except for chainreader before checking chainreader

orbeckst commented 4 months ago

@ljwoods2 instead of "WIP" in the title you can also "convert to draft" to make the state extra clear (and really properly tagged in GH instead using a convention). It also has the advantage that you then only need a click (instead of a title change) when you're ready.

ljwoods2 commented 4 months ago

Closing this because zarrtraj accepts filename strings instead of zarr.Group objects now, so no change to format_hint is needed