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

ENH: support pathlib in ReaderBase #3935

Merged

Conversation

tylerjereddy
Copy link
Member

  • the creation of a Universe object involving GROMACS formats should now accept pathlib
    objects as requested in Using pathlib Path object #2497; resolving that issue entirely is a separate matter, this branch focuses only on the GROMACS format and only if you proceed via the public Universe creation API

  • probably could make it work in Cython directly, though I'm not convinced we need to do that for this part of the fix at least, and we probably should still support strings as well of course

* the creation of a `Universe` object involving
GROMACS formats should now accept `pathlib`
objects as requested in MDAnalysisgh-2497; resolving that
issue entirely is a separate matter, this branch
focuses only on the GROMACS format and only if
you proceed via the public `Universe` creation
API

* probably could make it work in Cython directly,
though I'm not convinced we need to do that for
this part of the fix at least, and we probably
should still support strings as well of course
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a ReaderBase thing instead to store self.filename properly?

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Base: 94.36% // Head: 94.36% // Increases project coverage by +0.00% 🎉

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

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3935   +/-   ##
========================================
  Coverage    94.36%   94.36%           
========================================
  Files          194      194           
  Lines        25109    25115    +6     
  Branches      3398     3399    +1     
========================================
+ Hits         23694    23700    +6     
  Misses        1366     1366           
  Partials        49       49           
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/base.py 94.63% <100.00%> (+0.02%) ⬆️
MDAnalysisTests/coordinates/base.py 94.28% <0.00%> (+0.02%) ⬆️

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

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

@hmacdope
Copy link
Member

Shouldn't this be a ReaderBase thing instead to store self.filename properly?

Seconding this, can this not be dealt with at ReaderBase level to fix it for all readers?

* move the handling of `pathlib` objects
upstream to `ReaderBase` based on reviewer
feedback

* add one more format test related to this
@tylerjereddy
Copy link
Member Author

tylerjereddy commented Nov 24, 2022

Ok, I moved the patch to ReaderBase and added a test for one more format (DCD) as a sniff test for generalization.

I think that's probably good enough for now, but note that:

  1. This still doesn't cover all readers because SingleFrameReaderBase has its own self.filename -- I'd be happy to delay patching/testing that for now.
  2. The testing could probably be centralized/parametrized somewhere, covering a bunch of formats rather than testing the same pass-through for just two formats. Maybe good enough for now though..

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this passes tests I'm happy for this PR to go through. @tylerjereddy could you raise an issue about the missing bits? Maybe we should move stuff to ProtoReader in the future but it seems fine for this current issue?

@tylerjereddy
Copy link
Member Author

Eh, the failure is related--this is the problem with inheritance... I'd just as soon scope this back to GROMACS and move on.

I'll see if I can add a shim to fix it though.

* `NamedStream` gets special treatment (no string coercion)
in `ReaderBase`
@tylerjereddy
Copy link
Member Author

Ok, I pushed in a commit to shim around our NamedStream stuff in the base class, and this seems to allow the full suite to pass for me locally at least.

gh-3937 contains the the follow-up reminders I noted above.

@hmacdope
Copy link
Member

LGTM @tylerjereddy

@tylerjereddy
Copy link
Member Author

Ok, I have approvals from 3 core developers and CI is passing, in it goes.

@tylerjereddy tylerjereddy merged commit 5df4a55 into MDAnalysis:develop Nov 25, 2022
@tylerjereddy tylerjereddy deleted the treddy_issue_2497_gromacs branch November 25, 2022 00:27
@tylerjereddy tylerjereddy changed the title ENH: support GROMACS pathlib ENH: support pathlib in ReaderBase Nov 25, 2022
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

4 participants