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

MAINT, ENH: more pathlib support tasks #3937

Open
tylerjereddy opened this issue Nov 24, 2022 · 6 comments · May be fixed by #4535
Open

MAINT, ENH: more pathlib support tasks #3937

tylerjereddy opened this issue Nov 24, 2022 · 6 comments · May be fixed by #4535

Comments

@tylerjereddy
Copy link
Member

Some suggested follow-ups from gh-3935:

  1. Add support and testing for pathlib object handling to SingleFrameReaderBase and/or upstream this to the proto infrastructure. The pathlib handling/testing in the connected PR is restricted to ReaderBase for now.
  2. The testing added in the above PR might eventually benefit from being consolidated to i.e., universe creation over a parametrized set of trajectory formats instead of the copy-paste sampling I did for GRO and DCD formats.
@orbeckst
Copy link
Member

orbeckst commented Dec 1, 2022

And see also #2497

@orbeckst
Copy link
Member

orbeckst commented Feb 9, 2023

Is this issue sufficiently well defined to be suitable as a GSOC starter issue? Or could it be written in such a way that we can let GSOC applicants work on it?
(Also pinging reviewers of PR #3935 @hmacdope @richardjgowers @IAlibay and maybe @RMeli may be interested.)

@hmacdope hmacdope added this to the Release 3.0 milestone Oct 7, 2023
@hmacdope
Copy link
Member

hmacdope commented Oct 7, 2023

Adding this to 3.0

@talagayev
Copy link
Contributor

I had some time to take a look at this issue right now and from the looks of it and running some code SingleFrameReaderBase can currently handle both str and pathlib as an input. would it be enough to add tests, that showcase that SingleFrameReaderBase can handle both inputs correctly or is the upstream to proto and the addition of NamedStream still required?

@hmacdope
Copy link
Member

hmacdope commented Mar 26, 2024

I think the aim of this is to find any points we are loading files and support pathlib as well as a str. Best bet is to look for spots where files are loaded outside of ReaderBase objects. If there are no tests that cover this already though would welcome a PR adding tests. Unsure about NamedStream

@talagayev
Copy link
Contributor

If i understood correctly the idea is to see if SingleFrameReaderBase would need the same adjustments that were done for ReaderBase in PR #3935

with the case that is not covered by ReaderBase being the case of loading single frame formats, since they would be recognized with SingleFrameReaderBase. So the idea would be to use

u = mda.Universe(file.gro)

with file.gro being a single frame format with one frame that is recognized by SingleFrameReaderBase and test if it would recognize correctly a pathlib input as well as a str input for file.gro

currently the tests do test pathlib and str input for ReaderBase, but only test str input for SingleFrameReaderBase in the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants