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

Compatibility with Zarr Trajectory format #4557

Closed
wants to merge 2 commits into from

Conversation

ljwoods2
Copy link
Contributor

@ljwoods2 ljwoods2 commented Apr 3, 2024

Changes made in this Pull Request:

  • Modified the ChainedReader's _format_hint to not catch Zarr groups

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

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

Developers certificate of origin


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

Copy link

github-actions bot commented Apr 3, 2024

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!

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

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

Project coverage is 93.64%. Comparing base (73acc9b) to head (ee09673).

Files Patch % Lines
package/MDAnalysis/coordinates/chain.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             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              

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

@hmacdope
Copy link
Member

hmacdope commented Apr 8, 2024

@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
Copy link
Contributor Author

ljwoods2 commented Apr 8, 2024

@hmacdope Yes, Oliver mentioned this issue: #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
Copy link
Contributor Author

ljwoods2 commented Apr 8, 2024

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

@orbeckst
Copy link
Member

@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 ljwoods2 marked this pull request as draft May 31, 2024 01:03
@ljwoods2 ljwoods2 changed the title Compatibility with Zarr Trajectory format [WIP] Compatibility with Zarr Trajectory format May 31, 2024
@ljwoods2
Copy link
Contributor Author

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

@ljwoods2 ljwoods2 closed this May 31, 2024
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.

None yet

3 participants