From dc104740a407f6b6a3493d7a6f954f081cf5e86c Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 10 Aug 2023 10:45:03 -0700 Subject: [PATCH 1/7] fix error message for XDR readers - error message for not being able to write lockfile did not insert the filename into message: fixed by using proper f string - changed all .format to modern f string in XDR.py - add explicit test for offsets_filename() - add additional pytest.warns() to tests to catch warnings (reduces warnings output and checks that all expected warnings are raised with proper messages) --- package/MDAnalysis/coordinates/XDR.py | 17 +++++---- .../MDAnalysisTests/coordinates/test_xdr.py | 35 ++++++++++++++----- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index a3e15b2f35f..27d7f1efc49 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -63,8 +63,7 @@ def offsets_filename(filename, ending='npz'): """ head, tail = split(filename) - return join(head, '.{tail}_offsets.{ending}'.format(tail=tail, - ending=ending)) + return join(head, f'.{tail}_offsets.{ending}') def read_numpy_offsets(filename): @@ -88,7 +87,7 @@ def read_numpy_offsets(filename): # `ValueError` is encountered when the offset file is corrupted. except (ValueError, IOError): - warnings.warn("Failed to load offsets file {}\n".format(filename)) + warnings.warn(f"Failed to load offsets file {filename}\n") return False class XDRBaseReader(base.ReaderBase): @@ -201,7 +200,7 @@ def _load_offsets(self): except OSError as e: if isinstance(e, PermissionError) or e.errno == errno.EROFS: warnings.warn(f"Cannot write lock/offset file in same location as " - "{self.filename}. Using slow offset calculation.") + f"{self.filename}. Using slow offset calculation.") self._read_offsets(store=True) return else: @@ -220,10 +219,10 @@ def _load_offsets(self): # refer to Issue #1893 data = read_numpy_offsets(fname) if not data: - warnings.warn("Reading offsets from {} failed, " - "reading offsets from trajectory instead.\n" - "Consider setting 'refresh_offsets=True' " - "when loading your Universe.".format(fname)) + warnings.warn(f"Reading offsets from {fname} failed, " + f"reading offsets from trajectory instead.\n" + f"Consider setting 'refresh_offsets=True' " + f"when loading your Universe.") self._read_offsets(store=True) return @@ -256,7 +255,7 @@ def _read_offsets(self, store=False): offsets=offsets, size=size, ctime=ctime, n_atoms=self._xdr.n_atoms) except Exception as e: - warnings.warn("Couldn't save offsets because: {}".format(e)) + warnings.warn(f"Couldn't save offsets because: {e}") @property def n_frames(self): diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 315d3e14e68..76690adf802 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -23,6 +23,7 @@ import pytest from unittest.mock import patch +import re import os import shutil import subprocess @@ -46,6 +47,14 @@ from MDAnalysis.coordinates import XDR from MDAnalysisTests.util import get_userid +@pytest.mark.parametrize("filename,kwargs,reference", [ + ("foo.xtc", {}, ".foo.xtc_offsets.npz"), + ("foo.xtc", {"ending": "npz"}, ".foo.xtc_offsets.npz"), + ("bar.0001.trr", {"ending": "npzzzz"}, ".bar.0001.trr_offsets.npzzzz"), +]) +def test_offsets_filename(filename, kwargs, reference): + fn = XDR.offsets_filename(filename, **kwargs) + assert fn == reference class _XDRReader_Sub(object): @pytest.fixture() @@ -212,7 +221,7 @@ def go_beyond_EOF(): with pytest.raises(StopIteration): go_beyond_EOF() - + def test_read_next_timestep_ts_no_positions(self, universe): # primarily tests branching on ts.has_positions in _read_next_timestep ts = universe.trajectory[0] @@ -755,17 +764,21 @@ def test_nonexistent_offsets_file(self, traj): outfile_offsets = XDR.offsets_filename(traj) with patch.object(np, "load") as np_load_mock: np_load_mock.side_effect = IOError - saved_offsets = XDR.read_numpy_offsets(outfile_offsets) - assert_equal(saved_offsets, False) + with pytest.warns(UserWarning, match=re.escape( + f"Failed to load offsets file {outfile_offsets}")): + saved_offsets = XDR.read_numpy_offsets(outfile_offsets) + assert saved_offsets == False - def test_nonexistent_offsets_file(self, traj): + def test_corrupted_offsets_file(self, traj): # assert that a corrupted file returns False during read-in # Issue #3230 outfile_offsets = XDR.offsets_filename(traj) with patch.object(np, "load") as np_load_mock: np_load_mock.side_effect = ValueError - saved_offsets = XDR.read_numpy_offsets(outfile_offsets) - assert_equal(saved_offsets, False) + with pytest.warns(UserWarning, match=re.escape( + f"Failed to load offsets file {outfile_offsets}")): + saved_offsets = XDR.read_numpy_offsets(outfile_offsets) + assert saved_offsets == False def test_reload_offsets_if_offsets_readin_io_fails(self, trajectory): # force the np.load call that is called in read_numpy_offsets @@ -773,7 +786,12 @@ def test_reload_offsets_if_offsets_readin_io_fails(self, trajectory): # ensure that offsets are then read-in from the trajectory with patch.object(np, "load") as np_load_mock: np_load_mock.side_effect = IOError - trajectory._load_offsets() + with pytest.warns(UserWarning) as record: + trajectory._load_offsets() + assert len(record) == 2 + assert str(record[0].message).startswith("Failed to load offsets file") + assert "reading offsets from trajectory instead" in str(record[1].message) + assert_almost_equal( trajectory._xdr.offsets, self.ref_offsets, @@ -866,7 +884,8 @@ def test_unsupported_format(self, traj): np.savez(fname, **saved_offsets) # ok as long as this doesn't throw - reader = self._reader(traj) + with pytest.warns(UserWarning, match="Reload offsets from trajectory"): + reader = self._reader(traj) reader[idx_frame] @pytest.mark.skipif(get_userid() == 0, reason="cannot readonly as root") From 4f18afbb8e423386daf600cd6b13e56158e91eeb Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 10 Aug 2023 11:17:34 -0700 Subject: [PATCH 2/7] more robust checking of multiple warnings --- testsuite/MDAnalysisTests/coordinates/test_xdr.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 76690adf802..5c3852e358c 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -781,6 +781,9 @@ def test_corrupted_offsets_file(self, traj): assert saved_offsets == False def test_reload_offsets_if_offsets_readin_io_fails(self, trajectory): + # expected warning messages + warning_messages = ["Failed to load offsets file", + "reading offsets from trajectory instead"] # force the np.load call that is called in read_numpy_offsets # during _load_offsets to give an IOError # ensure that offsets are then read-in from the trajectory @@ -788,9 +791,15 @@ def test_reload_offsets_if_offsets_readin_io_fails(self, trajectory): np_load_mock.side_effect = IOError with pytest.warns(UserWarning) as record: trajectory._load_offsets() - assert len(record) == 2 - assert str(record[0].message).startswith("Failed to load offsets file") - assert "reading offsets from trajectory instead" in str(record[1].message) + + # check that all warning_messages have shown up + assert len(record) >= len(warning_messages) + found_messages = [str(r.message) for r in record] + # find at least 2 occurences (>= instead of == for robustness) + assert sum([ref in msg for msg in found_messages + for ref in warning_messages]) >= 2, ( + f"warning messages {warning_messages} " + f"not found in emitted messages {found_messages}") assert_almost_equal( trajectory._xdr.offsets, From 9b0c1c9a64977ccb75d599249390febd0c2b4049 Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 10 Aug 2023 11:18:24 -0700 Subject: [PATCH 3/7] Apply suggestions from code review Co-authored-by: Irfan Alibay --- testsuite/MDAnalysisTests/coordinates/test_xdr.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 5c3852e358c..aa13dd75d4e 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -47,6 +47,7 @@ from MDAnalysis.coordinates import XDR from MDAnalysisTests.util import get_userid + @pytest.mark.parametrize("filename,kwargs,reference", [ ("foo.xtc", {}, ".foo.xtc_offsets.npz"), ("foo.xtc", {"ending": "npz"}, ".foo.xtc_offsets.npz"), @@ -56,6 +57,7 @@ def test_offsets_filename(filename, kwargs, reference): fn = XDR.offsets_filename(filename, **kwargs) assert fn == reference + class _XDRReader_Sub(object): @pytest.fixture() def atoms(self): From 997e15c49a52474d888d254ccca10af0d29ebf49 Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 10 Aug 2023 12:00:32 -0700 Subject: [PATCH 4/7] simplified testing for multiple warnings --- .../MDAnalysisTests/coordinates/test_xdr.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 5c3852e358c..48896de43bc 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -781,26 +781,17 @@ def test_corrupted_offsets_file(self, traj): assert saved_offsets == False def test_reload_offsets_if_offsets_readin_io_fails(self, trajectory): - # expected warning messages - warning_messages = ["Failed to load offsets file", - "reading offsets from trajectory instead"] # force the np.load call that is called in read_numpy_offsets # during _load_offsets to give an IOError # ensure that offsets are then read-in from the trajectory with patch.object(np, "load") as np_load_mock: np_load_mock.side_effect = IOError - with pytest.warns(UserWarning) as record: + with (pytest.warns(UserWarning, + match="Failed to load offsets file") and + pytest.warns(UserWarning, + match="reading offsets from trajectory instead")): trajectory._load_offsets() - # check that all warning_messages have shown up - assert len(record) >= len(warning_messages) - found_messages = [str(r.message) for r in record] - # find at least 2 occurences (>= instead of == for robustness) - assert sum([ref in msg for msg in found_messages - for ref in warning_messages]) >= 2, ( - f"warning messages {warning_messages} " - f"not found in emitted messages {found_messages}") - assert_almost_equal( trajectory._xdr.offsets, self.ref_offsets, From 4f8be633cc06318bb3b0e5f6077f3dcefca205bc Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 10 Aug 2023 12:12:51 -0700 Subject: [PATCH 5/7] more explicit warnings testing - reduces warnings in test_xdr down to 2 (both related to del) - test for empty selection and missing elements warnings - rewriting more old-school parameter interpolation to f strings --- testsuite/MDAnalysisTests/coordinates/base.py | 12 +++++++----- .../MDAnalysisTests/coordinates/test_xdr.py | 16 ++++++++++------ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py index 9a684588917..67a62fb9c37 100644 --- a/testsuite/MDAnalysisTests/coordinates/base.py +++ b/testsuite/MDAnalysisTests/coordinates/base.py @@ -443,13 +443,13 @@ def test_pickle_reader(self, reader): assert_equal(len(reader), len(reader_p)) assert_equal(reader.ts, reader_p.ts, "Timestep is changed after pickling") - + def test_frame_collect_all_same(self, reader): - # check that the timestep resets so that the base reference is the same + # check that the timestep resets so that the base reference is the same # for all timesteps in a collection with the exception of memoryreader # and DCDReader if isinstance(reader, mda.coordinates.memory.MemoryReader): - pytest.xfail("memoryreader allows independent coordinates") + pytest.xfail("memoryreader allows independent coordinates") if isinstance(reader, mda.coordinates.DCD.DCDReader): pytest.xfail("DCDReader allows independent coordinates." "This behaviour is deprecated and will be changed" @@ -493,9 +493,11 @@ def test_timeseries_asel_shape(self, reader, asel): assert(timeseries.shape[0] == len(reader)) assert(timeseries.shape[1] == len(atoms)) assert(timeseries.shape[2] == 3) - + def test_timeseries_empty_asel(self, reader): - atoms = mda.Universe(reader.filename).select_atoms(None) + with pytest.warns(UserWarning, + match="Empty string to select atoms, empty group returned."): + atoms = mda.Universe(reader.filename).select_atoms(None) with pytest.raises(ValueError, match="Timeseries requires at least"): reader.timeseries(atoms) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 11f2533b62e..d9565b58976 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -553,7 +553,10 @@ class _GromacsWriterIssue117(object): @pytest.fixture() def universe(self): - return mda.Universe(PRMncdf, NCDF) + with pytest.warns(UserWarning, + match="ATOMIC_NUMBER record not found"): + u = mda.Universe(PRMncdf, NCDF) + return u def test_write_trajectory(self, universe, tmpdir): """Test writing Gromacs trajectories from AMBER NCDF (Issue 117)""" @@ -562,7 +565,9 @@ def test_write_trajectory(self, universe, tmpdir): for ts in universe.trajectory: W.write(universe) - uw = mda.Universe(PRMncdf, outfile) + with pytest.warns(UserWarning, + match="ATOMIC_NUMBER record not found"): + uw = mda.Universe(PRMncdf, outfile) # check that the coordinates are identical for each time step for orig_ts, written_ts in zip(universe.trajectory, uw.trajectory): @@ -570,10 +575,9 @@ def test_write_trajectory(self, universe, tmpdir): written_ts._pos, orig_ts._pos, self.prec, - err_msg="coordinate mismatch " - "between original and written " - "trajectory at frame %d (orig) vs %d " - "(written)" % (orig_ts.frame, written_ts.frame)) + err_msg=(f"coordinate mismatch between original and written " + f"trajectory at frame {orig_ts.frame:d} (orig) vs " + f"{orig_ts.frame:d} (written)")) class TestXTCWriterIssue117(_GromacsWriterIssue117): From f995b313e7afc95ad793808f389c79885731cba2 Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Thu, 10 Aug 2023 19:16:19 -0700 Subject: [PATCH 6/7] only use f-strings where used Co-authored-by: Rocco Meli --- package/MDAnalysis/coordinates/XDR.py | 6 +++--- testsuite/MDAnalysisTests/coordinates/test_xdr.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index 27d7f1efc49..0319f437ffa 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -220,9 +220,9 @@ def _load_offsets(self): data = read_numpy_offsets(fname) if not data: warnings.warn(f"Reading offsets from {fname} failed, " - f"reading offsets from trajectory instead.\n" - f"Consider setting 'refresh_offsets=True' " - f"when loading your Universe.") + "reading offsets from trajectory instead.\n" + "Consider setting 'refresh_offsets=True' " + "when loading your Universe.") self._read_offsets(store=True) return diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index d9565b58976..da009838e0e 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -575,7 +575,7 @@ def test_write_trajectory(self, universe, tmpdir): written_ts._pos, orig_ts._pos, self.prec, - err_msg=(f"coordinate mismatch between original and written " + err_msg=("coordinate mismatch between original and written " f"trajectory at frame {orig_ts.frame:d} (orig) vs " f"{orig_ts.frame:d} (written)")) From 874d9ba28f962a00fbece4d3f237f5bbd1561b99 Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Thu, 10 Aug 2023 19:17:07 -0700 Subject: [PATCH 7/7] ignore warnings not germane to the XDR tests --- testsuite/MDAnalysisTests/coordinates/test_xdr.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index d9565b58976..fc0a66f7f69 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -553,11 +553,9 @@ class _GromacsWriterIssue117(object): @pytest.fixture() def universe(self): - with pytest.warns(UserWarning, - match="ATOMIC_NUMBER record not found"): - u = mda.Universe(PRMncdf, NCDF) - return u + return mda.Universe(PRMncdf, NCDF) + @pytest.mark.filterwarnings("ignore: ATOMIC_NUMBER record not found") def test_write_trajectory(self, universe, tmpdir): """Test writing Gromacs trajectories from AMBER NCDF (Issue 117)""" outfile = str(tmpdir.join('xdr-writer-issue117' + self.ext)) @@ -565,9 +563,7 @@ def test_write_trajectory(self, universe, tmpdir): for ts in universe.trajectory: W.write(universe) - with pytest.warns(UserWarning, - match="ATOMIC_NUMBER record not found"): - uw = mda.Universe(PRMncdf, outfile) + uw = mda.Universe(PRMncdf, outfile) # check that the coordinates are identical for each time step for orig_ts, written_ts in zip(universe.trajectory, uw.trajectory):