From 9e89878bdca526effba58f7a436f97d2d3b28b92 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Wed, 12 Aug 2020 17:47:01 -0700 Subject: [PATCH 1/6] backported fix for DCD and XTC/TRR pickling - fix issue #2878 - add tests to the base tests (but xfail for any format that does not implement pickling yet) --- package/CHANGELOG | 3 +- package/MDAnalysis/lib/formats/libdcd.pyx | 3 +- package/MDAnalysis/lib/formats/libmdaxdr.pyx | 3 +- testsuite/MDAnalysisTests/coordinates/base.py | 40 ++++++++++++++++++- 4 files changed, 45 insertions(+), 4 deletions(-) diff --git a/package/CHANGELOG b/package/CHANGELOG index 24a6f94bc27..19fa7f7a1c5 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -13,7 +13,7 @@ The rules for this file: * release numbers follow "Semantic Versioning" http://semver.org ------------------------------------------------------------------------------ -??/??/20 orbeckst, VOD555, lilyminium +??/??/20 orbeckst, VOD555, lilyminium, yuxuanzhuang * 1.0.1 @@ -24,6 +24,7 @@ Fixes density=True; the keyword was available since 0.19.0 but with incorrect semantics and not documented and did not produce correct results (Issue #2811, PR #2812) + * pickle correct frames of DCD, TRR, and XTC (#2878) 06/09/20 richardjgowers, kain88-de, lilyminium, p-j-smith, bdice, joaomcteixeira, diff --git a/package/MDAnalysis/lib/formats/libdcd.pyx b/package/MDAnalysis/lib/formats/libdcd.pyx index e5d413c87e3..88fc75b83e8 100644 --- a/package/MDAnalysis/lib/formats/libdcd.pyx +++ b/package/MDAnalysis/lib/formats/libdcd.pyx @@ -265,7 +265,8 @@ cdef class DCDFile: return current_frame = state[1] - self.seek(current_frame) + self.seek(current_frame - 1) + self.current_frame = current_frame def tell(self): diff --git a/package/MDAnalysis/lib/formats/libmdaxdr.pyx b/package/MDAnalysis/lib/formats/libmdaxdr.pyx index c4a423d7141..4bcd763a42e 100644 --- a/package/MDAnalysis/lib/formats/libmdaxdr.pyx +++ b/package/MDAnalysis/lib/formats/libmdaxdr.pyx @@ -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 def seek(self, frame): """Seek to Frame. diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py index a2cb844e9af..1baff60c3c4 100644 --- a/testsuite/MDAnalysisTests/coordinates/base.py +++ b/testsuite/MDAnalysisTests/coordinates/base.py @@ -25,6 +25,7 @@ import numpy as np import pytest from six.moves import zip, range +from six.moves import cPickle as pickle from unittest import TestCase from numpy.testing import (assert_equal, assert_almost_equal, assert_array_almost_equal, assert_allclose) @@ -419,12 +420,22 @@ def test_transformations_copy(self,ref,transformed): ideal_coords = ref.iter_ts(i).positions + v1 + v2 assert_array_almost_equal(ts.positions, ideal_coords, decimal = ref.prec) - def test_add_another_transformations_raises_ValueError(self, transformed): # After defining the transformations, the workflow cannot be changed with pytest.raises(ValueError): transformed.add_transformations(translate([2,2,2])) + def test_pickle_reader(self, reader): + try: + reader_p = pickle.loads(pickle.dumps(reader)) + except (TypeError, ValueError): + pytest.xfail("Pickling not yet implemented for format {}".format( + reader.format)) + assert_equal(len(reader), len(reader_p)) + assert_equal(reader.ts, reader_p.ts, + "Timestep is changed after pickling") + + class MultiframeReaderTest(BaseReaderTest): def test_last_frame(self, ref, reader): ts = reader[-1] @@ -490,6 +501,33 @@ def test_iter_as_aux_lowf(self, ref, reader): ref.iter_ts(ref.aux_lowf_frames_with_steps[i]), decimal=ref.prec) + # To make sure we not only save the current timestep information, + # but also maintain its relative position. + def test_pickle_next_ts_reader(self, reader): + try: + reader_p = pickle.loads(pickle.dumps(reader)) + except (TypeError, ValueError): + pytest.xfail("Pickling not yet implemented for format {}".format( + reader.format)) + assert_equal(next(reader), next(reader_p), + "Next timestep is changed after pickling") + + # To make sure pickle works for last frame. + def test_pickle_last_ts_reader(self, reader): + # move current ts to last frame. + reader[-1] + try: + reader_p = pickle.loads(pickle.dumps(reader)) + except (TypeError, ValueError): + pytest.xfail("Pickling not yet implemented for format {}".format( + reader.format)) + assert_equal(len(reader), len(reader_p), + "Last timestep is changed after pickling") + assert_equal(reader.ts, reader_p.ts, + "Last timestep is changed after pickling") + + + class BaseWriterTest(object): @staticmethod From 79f252846f4952022a0b10ef2658a19d76721a0e Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Wed, 12 Aug 2020 17:51:46 -0700 Subject: [PATCH 2/6] increased release to 1.0.1-dev --- package/setup.py | 2 +- testsuite/setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package/setup.py b/package/setup.py index 61e7927db7f..6573bd028ee 100755 --- a/package/setup.py +++ b/package/setup.py @@ -73,7 +73,7 @@ from commands import getoutput # NOTE: keep in sync with MDAnalysis.__version__ in version.py -RELEASE = "1.0.0" +RELEASE = "1.0.1-dev" is_release = 'dev' not in RELEASE diff --git a/testsuite/setup.py b/testsuite/setup.py index 1616ef22d61..4a08841f84c 100755 --- a/testsuite/setup.py +++ b/testsuite/setup.py @@ -88,7 +88,7 @@ def run(self): if __name__ == '__main__': # this must be in-sync with MDAnalysis - RELEASE = "1.0.0" + RELEASE = "1.0.1-dev" with open("README") as summary: LONG_DESCRIPTION = summary.read() From ce8aa5f7b78a4a949a8146aa475bd3120dbc45fe Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 13 Aug 2020 10:37:50 -0700 Subject: [PATCH 3/6] tests for DCDFile pickling These tests all FAIL at the moment. --- testsuite/MDAnalysisTests/coordinates/base.py | 37 ------------------ .../MDAnalysisTests/formats/test_libdcd.py | 39 +++++++++++++++---- 2 files changed, 31 insertions(+), 45 deletions(-) diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py index 1baff60c3c4..d16073ffac5 100644 --- a/testsuite/MDAnalysisTests/coordinates/base.py +++ b/testsuite/MDAnalysisTests/coordinates/base.py @@ -25,7 +25,6 @@ import numpy as np import pytest from six.moves import zip, range -from six.moves import cPickle as pickle from unittest import TestCase from numpy.testing import (assert_equal, assert_almost_equal, assert_array_almost_equal, assert_allclose) @@ -425,16 +424,6 @@ def test_add_another_transformations_raises_ValueError(self, transformed): with pytest.raises(ValueError): transformed.add_transformations(translate([2,2,2])) - def test_pickle_reader(self, reader): - try: - reader_p = pickle.loads(pickle.dumps(reader)) - except (TypeError, ValueError): - pytest.xfail("Pickling not yet implemented for format {}".format( - reader.format)) - assert_equal(len(reader), len(reader_p)) - assert_equal(reader.ts, reader_p.ts, - "Timestep is changed after pickling") - class MultiframeReaderTest(BaseReaderTest): def test_last_frame(self, ref, reader): @@ -501,32 +490,6 @@ def test_iter_as_aux_lowf(self, ref, reader): ref.iter_ts(ref.aux_lowf_frames_with_steps[i]), decimal=ref.prec) - # To make sure we not only save the current timestep information, - # but also maintain its relative position. - def test_pickle_next_ts_reader(self, reader): - try: - reader_p = pickle.loads(pickle.dumps(reader)) - except (TypeError, ValueError): - pytest.xfail("Pickling not yet implemented for format {}".format( - reader.format)) - assert_equal(next(reader), next(reader_p), - "Next timestep is changed after pickling") - - # To make sure pickle works for last frame. - def test_pickle_last_ts_reader(self, reader): - # move current ts to last frame. - reader[-1] - try: - reader_p = pickle.loads(pickle.dumps(reader)) - except (TypeError, ValueError): - pytest.xfail("Pickling not yet implemented for format {}".format( - reader.format)) - assert_equal(len(reader), len(reader_p), - "Last timestep is changed after pickling") - assert_equal(reader.ts, reader_p.ts, - "Last timestep is changed after pickling") - - class BaseWriterTest(object): diff --git a/testsuite/MDAnalysisTests/formats/test_libdcd.py b/testsuite/MDAnalysisTests/formats/test_libdcd.py index 0028a80bd3b..7471df37e01 100644 --- a/testsuite/MDAnalysisTests/formats/test_libdcd.py +++ b/testsuite/MDAnalysisTests/formats/test_libdcd.py @@ -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) - assert dcd.fname == new_dcd.fname - assert dcd.tell() == new_dcd.tell() +def test_pickle(dcd): + 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): + 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)) + + 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): From c3d6b5933a25a790df60cc7f3bd25a3b48bbf342 Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 13 Aug 2020 10:58:51 -0700 Subject: [PATCH 4/6] tests for XTCFile pickling all pickle tests FAIL at the moment --- .../MDAnalysisTests/formats/test_libmdaxdr.py | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py b/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py index d4ed6190d2a..a054f9fa22a 100644 --- a/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py +++ b/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py @@ -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,20 +128,36 @@ 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() + + 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) + assert_almost_equal(frame.prec, new_frame.prec) + 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() From 9e98a55ea8d2a0d1810daf0defc3f4025bbf3926 Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 13 Aug 2020 12:02:31 -0700 Subject: [PATCH 5/6] removed prec from generic XDR pickle test --- testsuite/MDAnalysisTests/formats/test_libmdaxdr.py | 1 - 1 file changed, 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py b/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py index a054f9fa22a..c076a5130b8 100644 --- a/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py +++ b/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py @@ -141,7 +141,6 @@ def _assert_compare_readers(old_reader, new_reader): assert_almost_equal(frame.box, new_frame.box) assert frame.step == new_frame.step assert_almost_equal(frame.time, new_frame.time) - assert_almost_equal(frame.prec, new_frame.prec) def test_pickle(self, reader): mid = len(reader) // 2 From 6019c5e33f08029bc0433f823b30f344f0291831 Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 13 Aug 2020 12:22:39 -0700 Subject: [PATCH 6/6] add test for immediate pickling of xdr file --- testsuite/MDAnalysisTests/formats/test_libmdaxdr.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py b/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py index c076a5130b8..c1850ee25ac 100644 --- a/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py +++ b/testsuite/MDAnalysisTests/formats/test_libmdaxdr.py @@ -161,6 +161,16 @@ def test_pickle_closed(self, 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),