Trajectory slicing made completely Pythonic #918 (#1195)

Makes trajectory slicing completely pythonic #918 

Original commit messages:

* Trajectory slicing made completely pythonic #918
* Adds test cases
Fixes some logical error
* Merged develop branch with forked develop branch
* Adds test cases
Fixes some logical error
* Merged develop branch with forked develop branch
* Trajectory slicing made completely pythonic #918
* Adds test cases
Fixes some logical error
* Merged develop branch with forked develop branch
* Adds some more test cases for pythonic slicing
* Fixed minor bugs
* Reduced the lines of code for the pythonic slicing logic
The tests which no longer raises an Error are removed
The values for the removed tests are moved into the test generator.
* Fixes import order
* Refractored analysis/base.py
Added warning docstring to check_slice_indices
* Modifies AUTHORS and CHANGELOG

package/AUTHORS

@@ -81,6 +81,7 @@ Chronological list of authors
   - Shantanu Srivastava
  2017
   - Utkarsh Bansal
+  - Shobhit Agarwal
   - Vedant Rathore
 
 External code

package/CHANGELOG

@@ -15,7 +15,7 @@ The rules for this file:
 ------------------------------------------------------------------------------
 ??/??/16 kain88-de, fiona-naughton, richardjgowers, tyler.je.reddy, jdetle
          euhruska, orbeckst, rbrtdlg, jbarnoud, wouterboomsma, shanmbic,
-         dotsdl, manuel.nuno.melo, utkbansal, vedantrathore
+         dotsdl, manuel.nuno.melo, utkbansal, vedantrathore, shobhitagarwal1612
 
   * 0.16.0
 
@@ -61,6 +61,7 @@ Enhancements
   * Added groupby method to Group objects. (PR #1112)
 
 Fixes
+  * Trajectory slicing made completely Pythonic (Issue #918 PR #1195)
   * Argument validation of dist_mat_to_vec is fixed (#597 PR #1183)
   * Give correct error when the topology file format is not recognized (Issue #982)
   * Give correct error when file doesn't exist/ has bad permissions (Issue #981)

package/MDAnalysis/analysis/base.py

@@ -27,13 +27,12 @@
 classes.
 
 """
+import six
 from six.moves import range, zip
-
 import inspect
 import logging
-import numpy as np
-import six
 
+import numpy as np
 from MDAnalysis import coordinates
 from MDAnalysis.core.groups import AtomGroup
 from MDAnalysis.lib.log import ProgressMeter, _set_verbose
@@ -126,10 +125,10 @@ def _setup_frames(self, trajectory, start=None, stop=None, step=None):
             number of frames to skip between each analysed frame
         """
         self._trajectory = trajectory
-        start, stop, step = trajectory.check_slice_indices(start, stop, step)
         self.start = start
         self.stop = stop
         self.step = step
+        start, stop, step = trajectory.check_slice_indices(start, stop, step)
         self.n_frames = len(range(start, stop, step))
         interval = int(self.n_frames // 100)
         if interval == 0:
@@ -223,7 +222,7 @@ def __init__(self, function, trajectory=None, *args, **kwargs):
         """
         if (trajectory is not None) and (not isinstance(
                 trajectory, coordinates.base.Reader)):
-            args = args + (trajectory, )
+            args = args + (trajectory,)
             trajectory = None
 
         if trajectory is None:

package/MDAnalysis/analysis/contacts.py

@@ -212,12 +212,12 @@ def is_any_closer(r, r0, dist=2.5):
 
 """
 from __future__ import division
+from six.moves import zip
 
 import os
 import errno
 import warnings
 import bz2
-from six.moves import zip
 
 import numpy as np
 from numpy.lib.utils import deprecate

package/MDAnalysis/coordinates/base.py

@@ -123,29 +123,27 @@
 
 """
 from __future__ import absolute_import
-
-from six.moves import range
 import six
-
-import warnings
+from six.moves import range
 
 import numpy as np
 import copy
+import warnings
 import weakref
 
+from . import core
+from .. import NoDataError
 from .. import (
     _READERS,
     _SINGLEFRAME_WRITERS,
     _MULTIFRAME_WRITERS,
 )
-from ..core import flags
 from .. import units
-from ..lib.util import asiterable, Namespace
-from . import core
-from .. import NoDataError
-
 from ..auxiliary.base import AuxReader
 from ..auxiliary.core import auxreader
+from ..core import flags
+from ..lib.util import asiterable, Namespace
+
 
 class Timestep(object):
     """Timestep data for one frame
@@ -1191,6 +1189,7 @@ def __getitem__(self, frame):
         ----
         *frame* is a 0-based frame index.
         """
+
         def apply_limits(frame):
             if frame < 0:
                 frame += len(self)
@@ -1214,6 +1213,7 @@ def listiter(frames):
                     if not isinstance(f, (int, np.integer)):
                         raise TypeError("Frames indices must be integers")
                     yield self._read_frame_with_aux(apply_limits(f))
+
             return listiter(frame)
         elif isinstance(frame, slice):
             start, stop, step = self.check_slice_indices(
@@ -1277,6 +1277,21 @@ def check_slice_indices(self, start, stop, step):
         -------
         start, stop, step : int
           Integers representing the slice
+
+        Warning
+        -------
+        The returned values start, stop and step give the expected result when passed
+        in range() but gives unexpected behaviour when passed in a slice when stop=None
+        and step=-1
+
+        This is because when we slice the trajectory (u.trajectory[::-1]), the values
+        returned by check_slice_indices are passed to range. Instead, in AnalysisBase
+        the values returned by check_slice_indices are used to splice the trajectory.
+        This creates a discrepancy because these two lines are not equivalent:
+
+            range(10, -1, -1)  # [10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0]
+            range(10)[10:-1:-1]  # []
+
         """
         for var, varname in (
                 (start, 'start'),
@@ -1295,33 +1310,30 @@ def check_slice_indices(self, start, stop, step):
             start = 0 if step > 0 else nframes - 1
         elif start < 0:
             start += nframes
+        if start < 0:
+            start = 0
 
-        if stop is not None:
-            if stop < 0:
-                stop += nframes
-            elif stop > nframes:
-                stop = nframes
-        else:
+        if step < 0 and start > nframes:
+            start = nframes - 1
+
+        if stop is None:
             stop = nframes if step > 0 else -1
+        elif stop < 0:
+            stop += nframes
 
-        if step > 0 and stop < start:
-            raise IndexError("Stop frame is lower than start frame")
-        elif step < 0 and start < stop:
-            raise IndexError("Start frame is lower than stop frame")
-        if not (0 <= start < nframes) or stop > nframes:
-            raise IndexError(
-                "Frame start/stop outside of the range of the trajectory.")
+        if step > 0 and stop > nframes:
+            stop = nframes
 
         return start, stop, step
 
     def __repr__(self):
         return ("<{cls} {fname} with {nframes} frames of {natoms} atoms>"
                 "".format(
-                    cls=self.__class__.__name__,
-                    fname=self.filename,
-                    nframes=self.n_frames,
-                    natoms=self.n_atoms
-                ))
+            cls=self.__class__.__name__,
+            fname=self.filename,
+            nframes=self.n_frames,
+            natoms=self.n_atoms
+        ))
 
     def add_auxiliary(self, auxname, auxdata, format=None, **kwargs):
         """Add auxiliary data to be read alongside trajectory.
@@ -1421,7 +1433,7 @@ def next_as_aux(self, auxname):
         aux = self._check_for_aux(auxname)
         ts = self.ts
         # catch up auxiliary if it starts earlier than trajectory
-        while aux.step_to_frame(aux.step+1, ts) < 0:
+        while aux.step_to_frame(aux.step + 1, ts) < 0:
             next(aux)
         # find the next frame that'll have a representative value
         next_frame = aux.next_nonempty_frame(ts)
@@ -1555,7 +1567,6 @@ def rename_aux(self, auxname, new):
         setattr(self.ts.aux, new, self.ts.aux[auxname])
         delattr(self.ts.aux, auxname)
 
-
     def get_aux_descriptions(self, auxnames=None):
         """Get descriptions to allow reloading the specified auxiliaries.
 
@@ -1586,7 +1597,6 @@ def get_aux_descriptions(self, auxnames=None):
         return descriptions
 
 
-
 class Reader(ProtoReader):
     """Base class for trajectory readers that extends :class:`ProtoReader` with a
     :meth:`__del__` method.
@@ -1611,6 +1621,7 @@ class Reader(ProtoReader):
        Provides kwargs to be passed to :class:`Timestep`
 
     """
+
     def __init__(self, filename, convert_units=None, **kwargs):
         super(Reader, self).__init__()
 
@@ -1664,6 +1675,7 @@ class Writer(six.with_metaclass(_Writermeta, IObase)):
     See Trajectory API definition in :mod:`MDAnalysis.coordinates.__init__` for
     the required attributes and methods.
     """
+
     def convert_dimensions_to_unitcell(self, ts, inplace=True):
         """Read dimensions from timestep *ts* and return appropriate unitcell.
 
@@ -1730,7 +1742,7 @@ def has_valid_coordinates(self, criteria, x):
         x = np.ravel(x)
         return np.all(criteria["min"] < x) and np.all(x <= criteria["max"])
 
-    # def write_next_timestep(self, ts=None)
+        # def write_next_timestep(self, ts=None)
 
 
 class SingleFrameReader(ProtoReader):

package/MDAnalysis/lib/mdamath.py

@@ -38,8 +38,8 @@
 
 .. versionadded:: 0.11.0
 """
-import numpy as np
 from six.moves import zip
+import numpy as np
 
 from ..exceptions import NoDataError
 

package/MDAnalysis/lib/util.py

@@ -154,12 +154,12 @@
 .. versionchanged:: 0.11.0
    Moved mathematical functions into lib.mdamath
 """
+import six
+from six.moves import range, map
 import sys
 
 __docformat__ = "restructuredtext en"
 
-from six.moves import range, map
-import six
 
 import os
 import os.path

package/MDAnalysis/topology/TOPParser.py

@@ -70,8 +70,8 @@
 """
 from __future__ import absolute_import, division
 
-import numpy as np
 from six.moves import range, zip
+import numpy as np
 import functools
 from math import ceil
 

package/MDAnalysis/topology/XYZParser.py

@@ -39,8 +39,8 @@
 """
 from __future__ import absolute_import
 
-import numpy as np
 from six.moves import range
+import numpy as np
 
 from . import guessers
 from ..lib.util import openany

package/MDAnalysis/topology/tpr/utils.py

@@ -47,8 +47,8 @@
 """
 from __future__ import absolute_import
 
-import numpy as np
 from six.moves import range
+import numpy as np
 
 from . import obj
 from . import setting as S

testsuite/AUTHORS

@@ -73,6 +73,7 @@ Chronological list of authors
 
 2017
   - Utkarsh Bansal
+  - Shobhit Agarwal
   - Vedant Rathore
 
 External code