-
Notifications
You must be signed in to change notification settings - Fork 663
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
backport fix for #2878 (pickle DCD and XDR readers at correct frame) #2908
Changes from all commits
9e89878
79f2528
ce8aa5f
c3d6b59
9e98a55
6019c5e
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 | ||||
---|---|---|---|---|---|---|
|
@@ -265,7 +265,8 @@ cdef class DCDFile: | |||||
return | ||||||
|
||||||
current_frame = state[1] | ||||||
self.seek(current_frame) | ||||||
self.seek(current_frame - 1) | ||||||
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.
Suggested change
|
||||||
self.current_frame = current_frame | ||||||
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.
Suggested change
|
||||||
|
||||||
|
||||||
def tell(self): | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -306,7 +306,8 @@ cdef class _XDRFile: | |
|
||
# where was I | ||
current_frame = state[1] | ||
self.seek(current_frame) | ||
self.seek(current_frame - 1) | ||
self.current_frame = current_frame | ||
Comment on lines
+309
to
+310
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. Do these lines make the low-level tests fail? 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. The current tests can pass if these changes are ditched, but still the last frame cannot be easily pickled as |
||
|
||
def seek(self, frame): | ||
"""Seek to Frame. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,25 +84,48 @@ def dcd(): | |
with DCDFile(DCD) as dcd: | ||
yield dcd | ||
|
||
def _assert_compare_readers(old_reader, new_reader): | ||
frame = old_reader.read() # same as next(old_reader) | ||
new_frame = new_reader.read() # same as next(new_reader) | ||
|
||
def test_pickle(dcd): | ||
dcd.seek(len(dcd) - 1) | ||
dump = pickle.dumps(dcd) | ||
new_dcd = pickle.loads(dump) | ||
assert old_reader.fname == new_reader.fname | ||
assert old_reader.tell() == new_reader.tell() | ||
assert_almost_equal(frame.xyz, new_frame.xyz) | ||
assert_almost_equal(frame.unitcell, new_frame.unitcell) | ||
Comment on lines
+93
to
+94
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. added tests to check the contents of the current frame |
||
|
||
assert dcd.fname == new_dcd.fname | ||
assert dcd.tell() == new_dcd.tell() | ||
def test_pickle(dcd): | ||
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. make the generic pickle test start somewhere in the middle of the trajectory |
||
mid = len(dcd) // 2 | ||
dcd.seek(mid) | ||
new_dcd = pickle.loads(pickle.dumps(dcd)) | ||
_assert_compare_readers(dcd, new_dcd) | ||
|
||
def test_pickle_last(dcd): | ||
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. specific test for last frame (this was the original |
||
dcd.seek(len(dcd) - 1) | ||
new_dcd = pickle.loads(pickle.dumps(dcd)) | ||
_assert_compare_readers(dcd, new_dcd) | ||
|
||
def test_pickle_closed(dcd): | ||
dcd.seek(len(dcd) - 1) | ||
dcd.close() | ||
dump = pickle.dumps(dcd) | ||
new_dcd = pickle.loads(dump) | ||
new_dcd = pickle.loads(pickle.dumps(dcd)) | ||
|
||
assert dcd.fname == new_dcd.fname | ||
assert dcd.tell() != new_dcd.tell() | ||
|
||
def test_pickle_after_read(dcd): | ||
_ = dcd.read() | ||
new_dcd = pickle.loads(pickle.dumps(dcd)) | ||
_assert_compare_readers(dcd, new_dcd) | ||
|
||
#@pytest.mark.xfail | ||
def test_pickle_immediately(dcd): | ||
# do not seek before pickling: this seems to leave the DCDFile | ||
# object in weird state: is this supposed to work? | ||
new_dcd = pickle.loads(pickle.dumps(dcd)) | ||
Comment on lines
+121
to
+124
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. Pickle always fails if we have never done a
I find this behavior surprising (as a user) but maybe I don't understand if this is expected. But in that case we should raise an appropriate exception. 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. This should work. 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. Ok, thanks, then that's a bug. 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. Except that it is only a bug in this PR... because it works with 1.0.0 and current develop
For dcd
and also works for trr
EDIT: added loading and fixed DCD example 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. The approach I took here is flawed (so it also shouldn't work in current develop--when 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. Hm, I need to check what commit I used for this:
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. Oddly enough, >>> dcd = mda.lib.formats.libdcd.DCDFile(data.COORDINATES_DCD)
>>> read_p = pickle.loads(pickle.dumps(dcd))
>>> read_p.read()
OSError: Reading DCD header failed: format of DCD file is wrong |
||
|
||
assert dcd.fname == new_dcd.fname | ||
assert dcd.tell() == new_dcd.tell() | ||
|
||
|
||
@pytest.mark.parametrize("new_frame", (10, 42, 21)) | ||
def test_seek_normal(new_frame, dcd): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ | |
import numpy as np | ||
|
||
from numpy.testing import (assert_almost_equal, assert_array_almost_equal, | ||
assert_array_equal) | ||
assert_array_equal, assert_equal) | ||
|
||
from MDAnalysis.lib.formats.libmdaxdr import TRRFile, XTCFile | ||
|
||
|
@@ -128,24 +128,49 @@ def test_read_write_mode_file(self, xdr, tmpdir, fname): | |
with pytest.raises(IOError): | ||
f.read() | ||
|
||
@staticmethod | ||
def _assert_compare_readers(old_reader, new_reader): | ||
frame = old_reader.read() | ||
new_frame = new_reader.read() | ||
Comment on lines
+133
to
+134
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.
|
||
|
||
assert old_reader.fname == new_reader.fname | ||
assert old_reader.tell() == new_reader.tell() | ||
|
||
assert_equal(old_reader.offsets, new_reader.offsets) | ||
assert_almost_equal(frame.x, new_frame.x) | ||
assert_almost_equal(frame.box, new_frame.box) | ||
assert frame.step == new_frame.step | ||
assert_almost_equal(frame.time, new_frame.time) | ||
Comment on lines
+139
to
+143
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. added tests for contents of the frame |
||
|
||
def test_pickle(self, reader): | ||
reader.seek(len(reader) - 1) | ||
dump = pickle.dumps(reader) | ||
new_reader = pickle.loads(dump) | ||
mid = len(reader) // 2 | ||
reader.seek(mid) | ||
new_reader = pickle.loads(pickle.dumps(reader)) | ||
self._assert_compare_readers(reader, new_reader) | ||
|
||
assert reader.fname == new_reader.fname | ||
assert reader.tell() == new_reader.tell() | ||
assert_almost_equal(reader.offsets, new_reader.offsets) | ||
def test_pickle_last_frame(self, reader): | ||
reader.seek(len(reader) - 1) | ||
new_reader = pickle.loads(pickle.dumps(reader)) | ||
self._assert_compare_readers(reader, new_reader) | ||
|
||
def test_pickle_closed(self, reader): | ||
reader.seek(len(reader) - 1) | ||
reader.close() | ||
dump = pickle.dumps(reader) | ||
new_reader = pickle.loads(dump) | ||
new_reader = pickle.loads(pickle.dumps(reader)) | ||
|
||
assert reader.fname == new_reader.fname | ||
assert reader.tell() != new_reader.tell() | ||
|
||
#@pytest.mark.xfail | ||
def test_pickle_immediately(self, reader): | ||
# do not seek before pickling: this seems to leave the XDRFile | ||
# object in weird state: is this supposed to work? | ||
new_reader = pickle.loads(pickle.dumps(reader)) | ||
|
||
assert reader.fname == new_reader.fname | ||
assert reader.tell() == new_reader.tell() | ||
|
||
|
||
|
||
@pytest.mark.parametrize("xdrfile, fname, offsets", | ||
((XTCFile, XTC_multi_frame, XTC_OFFSETS), | ||
|
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.
Do these lines make the low-level tests fail?
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.
yeah...it messed up with the position of the file.
The fix is ditching current changes, and changing line 410 to:
Besides, I don't think current tests cover for the "true" last frame i.e.: