From 7a4aa9132ab66cbec04abc4897f0e3eddda9489f Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Fri, 28 Oct 2022 11:49:50 +1100
Subject: [PATCH 01/19] add timeseries back in

---
 package/MDAnalysis/coordinates/base.py | 73 ++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/package/MDAnalysis/coordinates/base.py b/package/MDAnalysis/coordinates/base.py
index 6c24602ecd9..d88e97b8f46 100644
--- a/package/MDAnalysis/coordinates/base.py
+++ b/package/MDAnalysis/coordinates/base.py
@@ -664,6 +664,8 @@ class ProtoReader(IOBase, metaclass=_Readermeta):
     #: The appropriate Timestep class, e.g.
     #: :class:`MDAnalysis.coordinates.xdrfile.XTC.Timestep` for XTC.
     _Timestep = Timestep
+    # the default coordinate layout for timeseries
+    _default_timeseries_order = 'fac'
 
     def __init__(self):
         # initialise list to store added auxiliary readers in
@@ -980,6 +982,77 @@ def __repr__(self):
             nframes=self.n_frames,
             natoms=self.n_atoms
         ))
+    
+    def timeseries(self, asel: Optional['AtomGroup']=None,
+                   start: Optional[int]=None, stop: Optional[int]=None,
+                   step: Optional[int]=None,
+                   order: Optional[str]='fac') -> np.ndarray:
+        """Return a subset of coordinate data for an AtomGroup
+        Parameters
+        ----------
+        asel : AtomGroup (optional)
+            The :class:`~MDAnalysis.core.groups.AtomGroup` to read the
+            coordinates from. Defaults to ``None``, in which case the full set of
+            coordinate data is returned.
+        start :  int (optional)
+             Begin reading the trajectory at frame index `start` (where 0 is the index
+             of the first frame in the trajectory); the default ``None`` starts
+             at the beginning.
+        stop : int (optional)
+             End reading the trajectory at frame index `stop`-1, i.e, `stop` is excluded.
+             The trajectory is read to the end with the default ``None``.
+        step : int (optional)
+             Step size for reading; the default ``None`` is equivalent to 1 and means to
+             read every frame.
+        order : str (optional)
+            the order/shape of the return data array, corresponding
+            to (a)tom, (f)rame, (c)oordinates all six combinations
+            of 'a', 'f', 'c' are allowed ie "fac" - return array
+            where the shape is (frame, number of atoms,
+            coordinates)
+        See Also
+        --------
+        MDAnalysis.coordinates.memory
+        .. versionadded:: 2.4.0
+        """
+        start, stop, step = self.check_slice_indices(start, stop, step)
+        nframes = len(range(start, stop, step))
+
+        if asel is not None:
+            if len(asel) == 0:
+                raise NoDataError(
+                    "Timeseries requires at least one atom to analyze")
+            atom_numbers = asel.indices
+            natoms = len(atom_numbers)
+        else:
+            natoms = self.n_atoms
+            atom_numbers = np.arange(natoms)
+
+        # allocate output array in 'fac' (_default_timeseries_order) order
+        coordinates = np.empty((nframes, natoms, 3), dtype=np.float32)
+        for i, ts in enumerate(self[start:stop:step]):
+            coordinates[i, :] = ts.positions[atom_numbers]
+
+        # switch axes around
+        if order == self._default_timeseries_order:
+            pass
+        elif order[0] == self._default_timeseries_order[0]:
+            coordinates = np.swapaxes(coordinates, 1, 2)
+        elif order[1] == self._default_timeseries_order[1]:
+            coordinates = np.swapaxes(coordinates, 0, 2)
+        elif order[2] == self._default_timeseries_order[2]:
+            coordinates = np.swapaxes(coordinates, 0, 1)
+        elif order[0] == self._default_timeseries_order[1]:
+            coordinates = np.swapaxes(coordinates, 1, 0)
+            coordinates = np.swapaxes(coordinates, 1, 2)
+        elif order[0] == self._default_timeseries_order[2]:
+            coordinates = np.swapaxes(coordinates, 2, 0)
+            coordinates = np.swapaxes(coordinates, 1, 2)
+        else:
+            raise ValueError("The order of the returned coordinate array must"
+                            " be a permutation of `fac`.")
+        return coordinates
+
 
 # TODO: Change order of aux_spec and auxdata for 3.0 release, cf. Issue #3811
     def add_auxiliary(self,

From db7a85488ba804262e24690cbdf7ef6f1936af5d Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Fri, 28 Oct 2022 12:52:37 +1100
Subject: [PATCH 02/19] add base tests

---
 testsuite/MDAnalysisTests/coordinates/base.py | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py
index 5fd4c77a444..d0a51a6867e 100644
--- a/testsuite/MDAnalysisTests/coordinates/base.py
+++ b/testsuite/MDAnalysisTests/coordinates/base.py
@@ -21,6 +21,7 @@
 # J. Comput. Chem. 32 (2011), 2319--2327, doi:10.1002/jcc.21787
 #
 import itertools
+from os import terminal_size
 import pickle
 
 import numpy as np
@@ -445,6 +446,29 @@ def test_pickle_reader(self, reader):
         assert_equal(reader.ts, reader_p.ts,
                      "Timestep is changed after pickling")
 
+    
+    @pytest.mark.parametrize('order', ('fac', 'fca', 'afc', 'acf', 'caf', 'cfa'))
+    def test_timeseries_shape(self, reader, order):
+        timeseries = reader.timeseries(order=order)
+        a_index = order.find('a')
+        f_index = order.find('f')
+        c_index = order.find('c')
+        assert(timeseries.shape[a_index] == reader.n_atoms)
+        assert(timeseries.shape[f_index] == len(reader))
+        assert(timeseries.shape[c_index] == 3)
+
+    @pytest.mark.parametrize('slice', ([0,2,1], [0,10,2], [0,10,3]))
+    def test_timeseries_values(self, reader, slice):
+        ts_positions = []
+        if slice[1] > len(reader):
+            pytest.xfail()
+        for i in range(slice[0], slice[1], slice[2]):
+            ts = reader[i]
+            ts_positions.append(ts.positions.copy())
+        positions = np.asarray(ts_positions)
+        timeseries = reader.timeseries(start=slice[0], stop=slice[1], step=slice[2], order='fac')
+        np.testing.assert_allclose(timeseries, positions)
+
 
 class MultiframeReaderTest(BaseReaderTest):
     def test_last_frame(self, ref, reader):

From e726579d10d2b0a6490925f7e8fcc25c4e6da0c3 Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Fri, 28 Oct 2022 14:25:03 +1100
Subject: [PATCH 03/19] add in test for order incorrect and test

---
 package/MDAnalysis/coordinates/base.py                   | 6 ++++--
 testsuite/MDAnalysisTests/coordinates/base.py            | 2 +-
 testsuite/MDAnalysisTests/coordinates/test_reader_api.py | 4 ++++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/package/MDAnalysis/coordinates/base.py b/package/MDAnalysis/coordinates/base.py
index d88e97b8f46..bf129ee6530 100644
--- a/package/MDAnalysis/coordinates/base.py
+++ b/package/MDAnalysis/coordinates/base.py
@@ -1018,6 +1018,9 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
         start, stop, step = self.check_slice_indices(start, stop, step)
         nframes = len(range(start, stop, step))
 
+        if sorted(order) != sorted(self._default_timeseries_order):
+            raise ValueError("The order of the returned coordinate array must"
+                            " be a permutation of `fac`.")
         if asel is not None:
             if len(asel) == 0:
                 raise NoDataError(
@@ -1049,8 +1052,7 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
             coordinates = np.swapaxes(coordinates, 2, 0)
             coordinates = np.swapaxes(coordinates, 1, 2)
         else:
-            raise ValueError("The order of the returned coordinate array must"
-                            " be a permutation of `fac`.")
+            pass
         return coordinates
 
 
diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py
index d0a51a6867e..7e9c6808ae3 100644
--- a/testsuite/MDAnalysisTests/coordinates/base.py
+++ b/testsuite/MDAnalysisTests/coordinates/base.py
@@ -461,7 +461,7 @@ def test_timeseries_shape(self, reader, order):
     def test_timeseries_values(self, reader, slice):
         ts_positions = []
         if slice[1] > len(reader):
-            pytest.xfail()
+            pytest.skip()
         for i in range(slice[0], slice[1], slice[2]):
             ts = reader[i]
             ts_positions.append(ts.positions.copy())
diff --git a/testsuite/MDAnalysisTests/coordinates/test_reader_api.py b/testsuite/MDAnalysisTests/coordinates/test_reader_api.py
index e57a9123a26..d7c965508ed 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_reader_api.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_reader_api.py
@@ -133,6 +133,10 @@ def test_raises_StopIteration(self, reader):
         with pytest.raises(StopIteration):
             next(reader)
 
+    @pytest.mark.parametrize('order', ['turnip', 'abc', ''])
+    def test_timeseries_raises_incorrect_order(self, reader, order):
+        with pytest.raises(ValueError, match="must be a permutation of `fac`"):
+            reader.timeseries(order=order)
 
 class _Multi(_TestReader):
     n_frames = 10

From 4431f1614a427f673946e87f92aac0761611b892 Mon Sep 17 00:00:00 2001
From: Hugo MacDermott-Opeskin <hugomacdermott@gmail.com>
Date: Sat, 29 Oct 2022 11:12:41 +1100
Subject: [PATCH 04/19] Apply suggestions from code review

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
---
 package/MDAnalysis/coordinates/base.py        | 40 +++++++++----------
 testsuite/MDAnalysisTests/coordinates/base.py | 14 +++----
 .../coordinates/test_reader_api.py            | 11 +++--
 3 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/package/MDAnalysis/coordinates/base.py b/package/MDAnalysis/coordinates/base.py
index bf129ee6530..2822c02384b 100644
--- a/package/MDAnalysis/coordinates/base.py
+++ b/package/MDAnalysis/coordinates/base.py
@@ -664,8 +664,6 @@ class ProtoReader(IOBase, metaclass=_Readermeta):
     #: The appropriate Timestep class, e.g.
     #: :class:`MDAnalysis.coordinates.xdrfile.XTC.Timestep` for XTC.
     _Timestep = Timestep
-    # the default coordinate layout for timeseries
-    _default_timeseries_order = 'fac'
 
     def __init__(self):
         # initialise list to store added auxiliary readers in
@@ -1010,17 +1008,17 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
             of 'a', 'f', 'c' are allowed ie "fac" - return array
             where the shape is (frame, number of atoms,
             coordinates)
+
         See Also
         --------
-        MDAnalysis.coordinates.memory
+        :class:`MDAnalysis.coordinates.memory`
+
+
         .. versionadded:: 2.4.0
         """
         start, stop, step = self.check_slice_indices(start, stop, step)
         nframes = len(range(start, stop, step))
 
-        if sorted(order) != sorted(self._default_timeseries_order):
-            raise ValueError("The order of the returned coordinate array must"
-                            " be a permutation of `fac`.")
         if asel is not None:
             if len(asel) == 0:
                 raise NoDataError(
@@ -1037,22 +1035,20 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
             coordinates[i, :] = ts.positions[atom_numbers]
 
         # switch axes around
-        if order == self._default_timeseries_order:
-            pass
-        elif order[0] == self._default_timeseries_order[0]:
-            coordinates = np.swapaxes(coordinates, 1, 2)
-        elif order[1] == self._default_timeseries_order[1]:
-            coordinates = np.swapaxes(coordinates, 0, 2)
-        elif order[2] == self._default_timeseries_order[2]:
-            coordinates = np.swapaxes(coordinates, 0, 1)
-        elif order[0] == self._default_timeseries_order[1]:
-            coordinates = np.swapaxes(coordinates, 1, 0)
-            coordinates = np.swapaxes(coordinates, 1, 2)
-        elif order[0] == self._default_timeseries_order[2]:
-            coordinates = np.swapaxes(coordinates, 2, 0)
-            coordinates = np.swapaxes(coordinates, 1, 2)
-        else:
-            pass
+        default_order = 'fac'
+        if order != default_order:
+            try:
+                newidx = [default_order.index(i) for i in order]
+            except ValueError:
+                raise ValueError(f"Unrecognized order key in {order}, "
+                                 "must be permutation of 'fac'")
+
+            try:
+                coordinates = np.moveaxis(coordinates, [0, 1, 2], newidx)
+            except ValueError:
+                errmsg = ("Repeated or missing keys passed to argument "
+                          f"`order`: {order}, each key must be used once")
+                raise ValueError(errmsg)
         return coordinates
 
 
diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py
index 7e9c6808ae3..64a9ac98356 100644
--- a/testsuite/MDAnalysisTests/coordinates/base.py
+++ b/testsuite/MDAnalysisTests/coordinates/base.py
@@ -21,7 +21,6 @@
 # J. Comput. Chem. 32 (2011), 2319--2327, doi:10.1002/jcc.21787
 #
 import itertools
-from os import terminal_size
 import pickle
 
 import numpy as np
@@ -450,9 +449,9 @@ def test_pickle_reader(self, reader):
     @pytest.mark.parametrize('order', ('fac', 'fca', 'afc', 'acf', 'caf', 'cfa'))
     def test_timeseries_shape(self, reader, order):
         timeseries = reader.timeseries(order=order)
-        a_index = order.find('a')
-        f_index = order.find('f')
-        c_index = order.find('c')
+        a_index = order.index('a')
+        f_index = order.index('f')
+        c_index = order.index('c')
         assert(timeseries.shape[a_index] == reader.n_atoms)
         assert(timeseries.shape[f_index] == len(reader))
         assert(timeseries.shape[c_index] == 3)
@@ -461,13 +460,14 @@ def test_timeseries_shape(self, reader, order):
     def test_timeseries_values(self, reader, slice):
         ts_positions = []
         if slice[1] > len(reader):
-            pytest.skip()
+            pytest.skip("too few frames in reader")
         for i in range(slice[0], slice[1], slice[2]):
             ts = reader[i]
             ts_positions.append(ts.positions.copy())
         positions = np.asarray(ts_positions)
-        timeseries = reader.timeseries(start=slice[0], stop=slice[1], step=slice[2], order='fac')
-        np.testing.assert_allclose(timeseries, positions)
+        timeseries = reader.timeseries(start=slice[0], stop=slice[1],
+                                       step=slice[2], order='fac')
+        assert_allclose(timeseries, positions)
 
 
 class MultiframeReaderTest(BaseReaderTest):
diff --git a/testsuite/MDAnalysisTests/coordinates/test_reader_api.py b/testsuite/MDAnalysisTests/coordinates/test_reader_api.py
index d7c965508ed..d9148e7fdd7 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_reader_api.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_reader_api.py
@@ -133,9 +133,14 @@ def test_raises_StopIteration(self, reader):
         with pytest.raises(StopIteration):
             next(reader)
 
-    @pytest.mark.parametrize('order', ['turnip', 'abc', ''])
-    def test_timeseries_raises_incorrect_order(self, reader, order):
-        with pytest.raises(ValueError, match="must be a permutation of `fac`"):
+    @pytest.mark.parametrize('order', ['turnip', 'abc'])
+    def test_timeseries_raises_unknown_order_key(self, reader, order):
+        with pytest.raises(ValueError, match="Unrecognized order key"):
+            reader.timeseries(order=order)
+
+    @pytest.mark.parametrize('order', ['faac', 'affc', 'afcc', ''])
+    def test_timeseries_raises_incorrect_order_key(self, reader, order):
+        with pytest.raises(ValueError, match="Repeated or missing keys"):
             reader.timeseries(order=order)
 
 class _Multi(_TestReader):

From 5bd24280d1614bc02b80225c482d1fcebf521e0b Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Sat, 29 Oct 2022 11:33:07 +1100
Subject: [PATCH 05/19] fix moveaxis

---
 package/MDAnalysis/coordinates/base.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/package/MDAnalysis/coordinates/base.py b/package/MDAnalysis/coordinates/base.py
index 2822c02384b..d13cb78a9f1 100644
--- a/package/MDAnalysis/coordinates/base.py
+++ b/package/MDAnalysis/coordinates/base.py
@@ -1039,12 +1039,13 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
         if order != default_order:
             try:
                 newidx = [default_order.index(i) for i in order]
+                print(newidx)
             except ValueError:
                 raise ValueError(f"Unrecognized order key in {order}, "
                                  "must be permutation of 'fac'")
 
             try:
-                coordinates = np.moveaxis(coordinates, [0, 1, 2], newidx)
+                coordinates = np.moveaxis(coordinates, newidx, [0, 1, 2])
             except ValueError:
                 errmsg = ("Repeated or missing keys passed to argument "
                           f"`order`: {order}, each key must be used once")

From b19be4600ac0d2767b9a528f07b11514b848d91d Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Sat, 29 Oct 2022 11:33:35 +1100
Subject: [PATCH 06/19] remove comment

---
 package/MDAnalysis/coordinates/base.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/package/MDAnalysis/coordinates/base.py b/package/MDAnalysis/coordinates/base.py
index d13cb78a9f1..07d1f1db42b 100644
--- a/package/MDAnalysis/coordinates/base.py
+++ b/package/MDAnalysis/coordinates/base.py
@@ -1039,7 +1039,6 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
         if order != default_order:
             try:
                 newidx = [default_order.index(i) for i in order]
-                print(newidx)
             except ValueError:
                 raise ValueError(f"Unrecognized order key in {order}, "
                                  "must be permutation of 'fac'")

From 3049ccbd10444ba5ad506a6525661dba80f6a302 Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Sat, 29 Oct 2022 11:39:48 +1100
Subject: [PATCH 07/19] skip test for memoryreader

---
 testsuite/MDAnalysisTests/coordinates/base.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py
index 64a9ac98356..26a9b087ed0 100644
--- a/testsuite/MDAnalysisTests/coordinates/base.py
+++ b/testsuite/MDAnalysisTests/coordinates/base.py
@@ -459,6 +459,8 @@ def test_timeseries_shape(self, reader, order):
     @pytest.mark.parametrize('slice', ([0,2,1], [0,10,2], [0,10,3]))
     def test_timeseries_values(self, reader, slice):
         ts_positions = []
+        if isinstance(reader, mda.coordinates.memory.MemoryReader):
+            pytest.xfail("MemoryReader uses deprecated indexing")
         if slice[1] > len(reader):
             pytest.skip("too few frames in reader")
         for i in range(slice[0], slice[1], slice[2]):

From a9e326ac8cce45f2465c22711c4ad2638c4f3391 Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Tue, 1 Nov 2022 10:39:12 +1100
Subject: [PATCH 08/19] add more detail to memoryreader xfail

---
 testsuite/MDAnalysisTests/coordinates/base.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py
index 26a9b087ed0..2da600b03a1 100644
--- a/testsuite/MDAnalysisTests/coordinates/base.py
+++ b/testsuite/MDAnalysisTests/coordinates/base.py
@@ -460,7 +460,8 @@ def test_timeseries_shape(self, reader, order):
     def test_timeseries_values(self, reader, slice):
         ts_positions = []
         if isinstance(reader, mda.coordinates.memory.MemoryReader):
-            pytest.xfail("MemoryReader uses deprecated indexing")
+            pytest.xfail("MemoryReader uses deprecated stop inclusive"
+                         " indexing, see Issue #3893")
         if slice[1] > len(reader):
             pytest.skip("too few frames in reader")
         for i in range(slice[0], slice[1], slice[2]):

From 020d8d7ac049fd31e5b5238f950be91623e1b8c8 Mon Sep 17 00:00:00 2001
From: Hugo MacDermott-Opeskin <hugomacdermott@gmail.com>
Date: Tue, 1 Nov 2022 11:42:15 +1100
Subject: [PATCH 09/19] Apply suggestions from code review

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
---
 package/MDAnalysis/coordinates/base.py        | 3 +--
 testsuite/MDAnalysisTests/coordinates/base.py | 2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/package/MDAnalysis/coordinates/base.py b/package/MDAnalysis/coordinates/base.py
index 07d1f1db42b..c47e3873a4d 100644
--- a/package/MDAnalysis/coordinates/base.py
+++ b/package/MDAnalysis/coordinates/base.py
@@ -1029,7 +1029,7 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
             natoms = self.n_atoms
             atom_numbers = np.arange(natoms)
 
-        # allocate output array in 'fac' (_default_timeseries_order) order
+        # allocate output array in 'fac' order
         coordinates = np.empty((nframes, natoms, 3), dtype=np.float32)
         for i, ts in enumerate(self[start:stop:step]):
             coordinates[i, :] = ts.positions[atom_numbers]
@@ -1051,7 +1051,6 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
                 raise ValueError(errmsg)
         return coordinates
 
-
 # TODO: Change order of aux_spec and auxdata for 3.0 release, cf. Issue #3811
     def add_auxiliary(self,
                       aux_spec: Union[str, Dict[str, str]] = None,
diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py
index 2da600b03a1..3cf059501ff 100644
--- a/testsuite/MDAnalysisTests/coordinates/base.py
+++ b/testsuite/MDAnalysisTests/coordinates/base.py
@@ -445,7 +445,6 @@ def test_pickle_reader(self, reader):
         assert_equal(reader.ts, reader_p.ts,
                      "Timestep is changed after pickling")
 
-    
     @pytest.mark.parametrize('order', ('fac', 'fca', 'afc', 'acf', 'caf', 'cfa'))
     def test_timeseries_shape(self, reader, order):
         timeseries = reader.timeseries(order=order)
@@ -472,7 +471,6 @@ def test_timeseries_values(self, reader, slice):
                                        step=slice[2], order='fac')
         assert_allclose(timeseries, positions)
 
-
 class MultiframeReaderTest(BaseReaderTest):
     def test_last_frame(self, ref, reader):
         ts = reader[-1]

From 5f1a2c1a6ae60c56558c65a2af130ff9da058a81 Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Tue, 1 Nov 2022 12:09:29 +1100
Subject: [PATCH 10/19] add test for empty asel

---
 testsuite/MDAnalysisTests/coordinates/base.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py
index 3cf059501ff..c3e591408a3 100644
--- a/testsuite/MDAnalysisTests/coordinates/base.py
+++ b/testsuite/MDAnalysisTests/coordinates/base.py
@@ -470,6 +470,11 @@ def test_timeseries_values(self, reader, slice):
         timeseries = reader.timeseries(start=slice[0], stop=slice[1],
                                        step=slice[2], order='fac')
         assert_allclose(timeseries, positions)
+    
+    def test_timeseries_empty_asel(self, reader):
+        atoms = mda.Universe(reader.filename).select_atoms(None)
+        with pytest.raises(NoDataError, match="Timeseries requires at least"):
+            reader.timeseries(atoms)
 
 class MultiframeReaderTest(BaseReaderTest):
     def test_last_frame(self, ref, reader):

From 5ca0a3af98880a13fe830460cc77d671c2d9a657 Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Tue, 1 Nov 2022 12:31:12 +1100
Subject: [PATCH 11/19] add timeseries empty warning

---
 package/MDAnalysis/coordinates/memory.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/package/MDAnalysis/coordinates/memory.py b/package/MDAnalysis/coordinates/memory.py
index 4995a633178..e18f68169b4 100644
--- a/package/MDAnalysis/coordinates/memory.py
+++ b/package/MDAnalysis/coordinates/memory.py
@@ -188,6 +188,7 @@
 import copy
 
 from . import base
+from .. import NoDataError
 from .timestep import Timestep
 
 
@@ -547,6 +548,9 @@ def timeseries(self, asel=None, start=0, stop=-1, step=1, order='afc'):
         if (asel is None or asel is asel.universe.atoms):
             return array
         else:
+            if len(asel) == 0:
+                raise NoDataError("Timeseries requires at least one atom "
+                                  "to analyze")
             # If selection is specified, return a copy
             return array.take(asel.indices, a_index)
 

From b5fa42d3c9ce7fb3d152fa00e9a8445429169f9c Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Tue, 1 Nov 2022 14:06:51 +1100
Subject: [PATCH 12/19] fix docs

---
 package/MDAnalysis/coordinates/base.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/package/MDAnalysis/coordinates/base.py b/package/MDAnalysis/coordinates/base.py
index c47e3873a4d..d6529ff6bd3 100644
--- a/package/MDAnalysis/coordinates/base.py
+++ b/package/MDAnalysis/coordinates/base.py
@@ -986,6 +986,7 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
                    step: Optional[int]=None,
                    order: Optional[str]='fac') -> np.ndarray:
         """Return a subset of coordinate data for an AtomGroup
+        
         Parameters
         ----------
         asel : AtomGroup (optional)
@@ -993,15 +994,15 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
             coordinates from. Defaults to ``None``, in which case the full set of
             coordinate data is returned.
         start :  int (optional)
-             Begin reading the trajectory at frame index `start` (where 0 is the index
-             of the first frame in the trajectory); the default ``None`` starts
-             at the beginning.
+            Begin reading the trajectory at frame index `start` (where 0 is the index
+            of the first frame in the trajectory); the default ``None`` starts
+            at the beginning.
         stop : int (optional)
-             End reading the trajectory at frame index `stop`-1, i.e, `stop` is excluded.
-             The trajectory is read to the end with the default ``None``.
+            End reading the trajectory at frame index `stop`-1, i.e, `stop` is excluded.
+            The trajectory is read to the end with the default ``None``.
         step : int (optional)
-             Step size for reading; the default ``None`` is equivalent to 1 and means to
-             read every frame.
+            Step size for reading; the default ``None`` is equivalent to 1 and means to
+            read every frame.
         order : str (optional)
             the order/shape of the return data array, corresponding
             to (a)tom, (f)rame, (c)oordinates all six combinations

From 89c4beefb8ea76c6787c2e0f3213c020863c4b1e Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Tue, 1 Nov 2022 14:10:07 +1100
Subject: [PATCH 13/19] fix indentation of docs

---
 package/MDAnalysis/coordinates/base.py | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/package/MDAnalysis/coordinates/base.py b/package/MDAnalysis/coordinates/base.py
index d6529ff6bd3..c6ec9523c3c 100644
--- a/package/MDAnalysis/coordinates/base.py
+++ b/package/MDAnalysis/coordinates/base.py
@@ -986,23 +986,24 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
                    step: Optional[int]=None,
                    order: Optional[str]='fac') -> np.ndarray:
         """Return a subset of coordinate data for an AtomGroup
-        
+
         Parameters
         ----------
         asel : AtomGroup (optional)
             The :class:`~MDAnalysis.core.groups.AtomGroup` to read the
-            coordinates from. Defaults to ``None``, in which case the full set of
-            coordinate data is returned.
+            coordinates from. Defaults to ``None``, in which case the full set
+            of coordinate data is returned.
         start :  int (optional)
-            Begin reading the trajectory at frame index `start` (where 0 is the index
-            of the first frame in the trajectory); the default ``None`` starts
-            at the beginning.
+            Begin reading the trajectory at frame index `start` (where 0 is the
+            index of the first frame in the trajectory); the default
+            ``None`` starts at the beginning.
         stop : int (optional)
-            End reading the trajectory at frame index `stop`-1, i.e, `stop` is excluded.
-            The trajectory is read to the end with the default ``None``.
+            End reading the trajectory at frame index `stop`-1, i.e, `stop` is
+            excluded. The trajectory is read to the end with the default
+            ``None``.
         step : int (optional)
-            Step size for reading; the default ``None`` is equivalent to 1 and means to
-            read every frame.
+            Step size for reading; the default ``None`` is equivalent to 1 and
+            means to read every frame.
         order : str (optional)
             the order/shape of the return data array, corresponding
             to (a)tom, (f)rame, (c)oordinates all six combinations

From 3d614ff27ecc9465a14c250cb6a9087f7a96ae47 Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Thu, 3 Nov 2022 11:31:14 +1100
Subject: [PATCH 14/19] add test for atom selection in base.timeseries

---
 testsuite/MDAnalysisTests/coordinates/base.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py
index c3e591408a3..d5c27134900 100644
--- a/testsuite/MDAnalysisTests/coordinates/base.py
+++ b/testsuite/MDAnalysisTests/coordinates/base.py
@@ -470,6 +470,14 @@ def test_timeseries_values(self, reader, slice):
         timeseries = reader.timeseries(start=slice[0], stop=slice[1],
                                        step=slice[2], order='fac')
         assert_allclose(timeseries, positions)
+
+    @pytest.mark.parametrize('asel', ("index 1", "index 2", "index 1 to 3"))
+    def test_timeseries_asel_shape(self, reader, asel):
+        atoms = mda.Universe(reader.filename).select_atoms(asel)
+        timeseries = reader.timeseries(atoms, order='fac')
+        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)

From 29f2b3d8c1d7337e914d46f4a2724c79d3a268ce Mon Sep 17 00:00:00 2001
From: Hugo Macdermott <hugomacdermott@Hugos-MacBook-Air-2.local>
Date: Sun, 6 Nov 2022 14:36:15 +1100
Subject: [PATCH 15/19] update empty asel to be ValueError rather than
 NoDataError

---
 package/CHANGELOG                             | 3 +++
 package/MDAnalysis/coordinates/DCD.py         | 3 +--
 package/MDAnalysis/coordinates/base.py        | 3 +--
 package/MDAnalysis/coordinates/memory.py      | 6 ++++--
 testsuite/MDAnalysisTests/coordinates/base.py | 3 +--
 5 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/package/CHANGELOG b/package/CHANGELOG
index 2c85bb9777f..2e45feaa5fc 100644
--- a/package/CHANGELOG
+++ b/package/CHANGELOG
@@ -19,6 +19,8 @@ The rules for this file:
  * 2.4.0
 
 Fixes
+  * NoDataErrors raised on empty atomgroup provided to timeseries in DCDReader and MemoryReader
+    were changed to ValueError as NoDataError is intended for TopologyAttrs (cf #3901)
   * LAMMPSDump Reader translates the box to the origin (#3741)
   * hbond analysis: access hbonds results through new results member in count_by_ids() and count_by_type()
   * Added isolayer selection method (Issue #3845)
@@ -31,6 +33,7 @@ Fixes
     (e.g. bonds, angles) (PR #3779).
 
 Enhancements
+  * The timeseries method for dumping coordinates to a NumPy array was added to ProtoReader
   * LAMMPSDump Reader optionally unwraps trajectories with image flags upon 
     loading (#3843
   * LAMMPSDump Reader now imports velocities and forces (#3843)
diff --git a/package/MDAnalysis/coordinates/DCD.py b/package/MDAnalysis/coordinates/DCD.py
index 3b340b78917..40efb7639fd 100644
--- a/package/MDAnalysis/coordinates/DCD.py
+++ b/package/MDAnalysis/coordinates/DCD.py
@@ -64,7 +64,6 @@
 import warnings
 
 from .. import units as mdaunits  # use mdaunits instead of units to avoid a clash
-from ..exceptions import NoDataError
 from . import base, core
 from ..lib.formats.libdcd import DCDFile
 from ..lib.mdamath import triclinic_box
@@ -304,7 +303,7 @@ def timeseries(self,
 
         if asel is not None:
             if len(asel) == 0:
-                raise NoDataError(
+                raise ValueError(
                     "Timeseries requires at least one atom to analyze")
             atom_numbers = list(asel.indices)
         else:
diff --git a/package/MDAnalysis/coordinates/base.py b/package/MDAnalysis/coordinates/base.py
index c6ec9523c3c..9a707030f76 100644
--- a/package/MDAnalysis/coordinates/base.py
+++ b/package/MDAnalysis/coordinates/base.py
@@ -131,7 +131,6 @@
 
 from .timestep import Timestep
 from . import core
-from .. import NoDataError
 from .. import (
     _READERS, _READER_HINTS,
     _SINGLEFRAME_WRITERS,
@@ -1023,7 +1022,7 @@ def timeseries(self, asel: Optional['AtomGroup']=None,
 
         if asel is not None:
             if len(asel) == 0:
-                raise NoDataError(
+                raise ValueError(
                     "Timeseries requires at least one atom to analyze")
             atom_numbers = asel.indices
             natoms = len(atom_numbers)
diff --git a/package/MDAnalysis/coordinates/memory.py b/package/MDAnalysis/coordinates/memory.py
index 9eca9ded7e0..8c2b459128d 100644
--- a/package/MDAnalysis/coordinates/memory.py
+++ b/package/MDAnalysis/coordinates/memory.py
@@ -188,7 +188,6 @@
 import copy
 
 from . import base
-from .. import NoDataError
 from .timestep import Timestep
 
 
@@ -522,6 +521,9 @@ def timeseries(self, asel=None, start=0, stop=-1, step=1, order='afc'):
 
         .. versionchanged:: 1.0.0
            Deprecated `format` keyword has been removed. Use `order` instead.
+        .. versionchanged:: 2.4.0
+            ValueError now raised instead of NoDataError for empty input
+            AtomGroup
         """
         if stop != -1:
             warnings.warn("MemoryReader.timeseries inclusive `stop` "
@@ -561,7 +563,7 @@ def timeseries(self, asel=None, start=0, stop=-1, step=1, order='afc'):
             return array
         else:
             if len(asel) == 0:
-                raise NoDataError("Timeseries requires at least one atom "
+                raise ValueError("Timeseries requires at least one atom "
                                   "to analyze")
             # If selection is specified, return a copy
             return array.take(asel.indices, a_index)
diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py
index d5c27134900..23eeb6bc04a 100644
--- a/testsuite/MDAnalysisTests/coordinates/base.py
+++ b/testsuite/MDAnalysisTests/coordinates/base.py
@@ -32,7 +32,6 @@
 import MDAnalysis as mda
 from MDAnalysis.coordinates.timestep import Timestep
 from MDAnalysis.transformations import translate
-from MDAnalysis import NoDataError
 
 
 from MDAnalysisTests.coordinates.reference import RefAdKSmall
@@ -481,7 +480,7 @@ def test_timeseries_asel_shape(self, reader, asel):
     
     def test_timeseries_empty_asel(self, reader):
         atoms = mda.Universe(reader.filename).select_atoms(None)
-        with pytest.raises(NoDataError, match="Timeseries requires at least"):
+        with pytest.raises(ValueError, match="Timeseries requires at least"):
             reader.timeseries(atoms)
 
 class MultiframeReaderTest(BaseReaderTest):

From f55c2f19c020c47f85eed7cf89adc9ac7c86349b Mon Sep 17 00:00:00 2001
From: Hugo MacDermott <hugomacdermott@gmail.com>
Date: Sun, 6 Nov 2022 14:43:57 +1100
Subject: [PATCH 16/19] remove DCD timeseries NoDataError tests

---
 testsuite/MDAnalysisTests/coordinates/test_dcd.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/testsuite/MDAnalysisTests/coordinates/test_dcd.py b/testsuite/MDAnalysisTests/coordinates/test_dcd.py
index d5e93b1fac7..6c94485607f 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_dcd.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_dcd.py
@@ -24,7 +24,6 @@
 
 import MDAnalysis as mda
 from MDAnalysis.coordinates.DCD import DCDReader
-from MDAnalysis.exceptions import NoDataError
 
 from numpy.testing import (assert_equal, assert_array_equal,
                            assert_almost_equal, assert_array_almost_equal)
@@ -231,7 +230,7 @@ def test_timeseries_atomindices(indices, universe_dcd):
 
 
 def test_timeseries_empty_selection(universe_dcd):
-    with pytest.raises(NoDataError):
+    with pytest.raises(ValueError):
         asel = universe_dcd.select_atoms('name FOO')
         universe_dcd.trajectory.timeseries(asel=asel)
 

From eed3029d04fbefc54d3b468209e1975923ca6d2f Mon Sep 17 00:00:00 2001
From: Hugo MacDermott <hugomacdermott@gmail.com>
Date: Sun, 6 Nov 2022 14:48:46 +1100
Subject: [PATCH 17/19] add versionchanged to DCD

---
 package/MDAnalysis/coordinates/DCD.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/package/MDAnalysis/coordinates/DCD.py b/package/MDAnalysis/coordinates/DCD.py
index 40efb7639fd..36a1571766f 100644
--- a/package/MDAnalysis/coordinates/DCD.py
+++ b/package/MDAnalysis/coordinates/DCD.py
@@ -297,6 +297,9 @@ def timeseries(self,
 
         .. versionchanged:: 1.0.0
            `skip` and `format` keywords have been removed.
+        .. versionchanged:: 2.4.0
+            ValueError now raised instead of NoDataError for empty input
+            AtomGroup
         """
 
         start, stop, step = self.check_slice_indices(start, stop, step)

From b268dd1cdeab9969f46580b3aa563fab43970b9b Mon Sep 17 00:00:00 2001
From: Hugo MacDermott-Opeskin <hugomacdermott@gmail.com>
Date: Mon, 7 Nov 2022 09:31:44 +1100
Subject: [PATCH 18/19] Update package/CHANGELOG

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
---
 package/CHANGELOG | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/package/CHANGELOG b/package/CHANGELOG
index 2e45feaa5fc..07bebb80354 100644
--- a/package/CHANGELOG
+++ b/package/CHANGELOG
@@ -33,7 +33,8 @@ Fixes
     (e.g. bonds, angles) (PR #3779).
 
 Enhancements
-  * The timeseries method for dumping coordinates to a NumPy array was added to ProtoReader
+  * The timeseries method for dumping coordinates to a NumPy array was added
+    to ProtoReader (PR #3890)
   * LAMMPSDump Reader optionally unwraps trajectories with image flags upon 
     loading (#3843
   * LAMMPSDump Reader now imports velocities and forces (#3843)

From a2e6d8bee9e9006515ee98294e2a12a3755bdb80 Mon Sep 17 00:00:00 2001
From: hmacdope <hugomacdermott@gmail.com>
Date: Mon, 7 Nov 2022 09:55:16 +1100
Subject: [PATCH 19/19] fix innaccurate changelog changes

---
 package/CHANGELOG | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/package/CHANGELOG b/package/CHANGELOG
index 07bebb80354..77dfd16e3ac 100644
--- a/package/CHANGELOG
+++ b/package/CHANGELOG
@@ -19,8 +19,9 @@ The rules for this file:
  * 2.4.0
 
 Fixes
-  * NoDataErrors raised on empty atomgroup provided to timeseries in DCDReader and MemoryReader
-    were changed to ValueError as NoDataError is intended for TopologyAttrs (cf #3901)
+  * NoDataError raised on empty atomgroup provided to `DCDReader.timeseries` 
+    was changed to ValueError (see #3901). A matching ValueError was added to
+    `MemoryReader.timeseries`(PR #3890). 
   * LAMMPSDump Reader translates the box to the origin (#3741)
   * hbond analysis: access hbonds results through new results member in count_by_ids() and count_by_type()
   * Added isolayer selection method (Issue #3845)
@@ -33,8 +34,8 @@ Fixes
     (e.g. bonds, angles) (PR #3779).
 
 Enhancements
-  * The timeseries method for dumping coordinates to a NumPy array was added
-    to ProtoReader (PR #3890)
+  * The timeseries method for exporting coordinates from multiple frames to a
+    NumPy array was added to ProtoReader (PR #3890)
   * LAMMPSDump Reader optionally unwraps trajectories with image flags upon 
     loading (#3843
   * LAMMPSDump Reader now imports velocities and forces (#3843)