-
Notifications
You must be signed in to change notification settings - Fork 652
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
Pathlib object handling for Universe, SingleFrameReaderBase and Toplogy parsers (Issue #3937) #4535
base: develop
Are you sure you want to change the base?
Changes from all commits
0dbca13
c2c7c2c
05ea8de
5c0383c
9a4932b
ff2a832
d85defc
87f7a95
27e9685
567e6ef
5558dda
2953310
c181021
e06ae55
0aa40cc
5dafd01
33d6b4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1662,7 +1662,11 @@ class SingleFrameReaderBase(ProtoReader): | |
def __init__(self, filename, convert_units=True, n_atoms=None, **kwargs): | ||
super(SingleFrameReaderBase, self).__init__() | ||
|
||
self.filename = filename | ||
if isinstance(filename, NamedStream): | ||
self.filename = filename | ||
else: | ||
self.filename = str(filename) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here errors if we use
I managed to fix the error by adding: basically putting a specific condition for MMTF cases, not the cleanest approach, but maybe this helps @hmacdope fixing the error in a better way :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to look more into it now, the
This helps to fix the errors, although it is quite similar as to just directly using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @talagayev I think the if isinstance(filename, NamedStream):
self.filename = filename
elif 'mmtf' in str(filename.__class__):
# mmtf case, where we've avoided a mmtf import in detection
self.filename = filename Where you've enumerated all the special cases explicitly in each There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see, yes the option that I got worked, but was definitely not the best option, but yes was connected with mmtf and yes |
||
|
||
self.convert_units = convert_units | ||
|
||
self.n_frames = 1 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,12 +25,14 @@ | |
|
||
import numpy as np | ||
import pytest | ||
from pathlib import Path | ||
from unittest import TestCase | ||
from numpy.testing import (assert_equal, assert_almost_equal, | ||
assert_array_almost_equal, assert_allclose) | ||
|
||
import MDAnalysis as mda | ||
from MDAnalysis.coordinates.timestep import Timestep | ||
from MDAnalysis.coordinates.memory import MemoryReader | ||
from MDAnalysis.transformations import translate | ||
|
||
|
||
|
@@ -134,6 +136,12 @@ def test_pickle_singleframe_reader(self): | |
assert_equal(reader.ts, reader_p.ts, | ||
"Modification of ts not preserved after serialization") | ||
|
||
def test_pathlib_input_single(self): | ||
path = Path(self.filename) | ||
u_str = mda.Universe(self.filename) | ||
u_path = mda.Universe(path) | ||
assert u_str.atoms.n_atoms == u_path.atoms.n_atoms | ||
|
||
|
||
class BaseReference(object): | ||
def __init__(self): | ||
|
@@ -537,6 +545,16 @@ def test_timeseries_atomgroup_asel_mutex(self, reader): | |
atoms = mda.Universe(reader.filename).select_atoms("index 1") | ||
with pytest.raises(ValueError, match="Cannot provide both"): | ||
timeseries = reader.timeseries(atomgroup=atoms, asel=atoms, order='fac') | ||
|
||
def test_pathlib_input_base(self, reader): | ||
if isinstance(reader, MemoryReader): | ||
if isinstance(reader, MemoryReader): | ||
skip_reason = "MemoryReader" | ||
pytest.skip(f"Skipping test for Pathlib input with reason: {skip_reason}") | ||
path = Path(reader.filename) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here adding an additional Line: Fixes the following Failed Pytest in 4 cases: |
||
u_str = mda.Universe(reader.filename) | ||
u_path = mda.Universe(path) | ||
assert u_str.atoms.n_atoms == u_path.atoms.n_atoms | ||
|
||
|
||
class MultiframeReaderTest(BaseReaderTest): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,6 @@ | |
assert_equal, | ||
) | ||
import pytest | ||
from io import StringIO | ||
|
||
|
||
class TestGROReaderOld(RefAdK): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit vague and doesn't actually tell me what is fixed or what is now possible. e.g. Is it that
pathlib.Path
s are now correctly handled?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, originally it was only targeting pathlib object handling of SingleFrameReaderBase in specific cases, but with the adjustment of @hmacdope it would cover
Pathlib.Path
s for all cases if the errros that appear currently due to the changes in thebase.py
in topology and also thebase.py
in coordinates, which could be fixed withelif hasattr(filename, 'topology'):
self.filename = filename
but this would just exclude the cases of
topology
so I am not sure if this would fix it and cover then all thePathlib.Path
cases