-
Notifications
You must be signed in to change notification settings - Fork 643
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
Trajectory slicing should be completely Pythonic #918
Comments
So more a question, trajectory slicing should always just work like slicing a list right? |
The common trick is to actually slice a list or array like you did in your example and then fancy-index the trajectory... In principle we're doing this already in |
And yes, I am also of the opinion that trajectory slicing should just behave like array slicing and indexing. |
How about def get_slice_frame_indices(self, trjslice):
frame_indices = np.arange(self.n_frames)[trjslice]
if not frame_indices:
raise IndexError("Slice {0} results in an empty trajectory.".format(trjslice))
return frame_indices
def _sliced_iter(self, start, stop, step):
for i in self.get_slice_frame_indices(slice(start, stop, step)):
yield self._read_frame(i) (without docs and defensive programming but you should get the idea...) |
That can be quite a huge array. It will be slow to build, and memory costly. Removing that kind of constructs in the auxiliary readers improved the perf by 2 orders of magnitude, I worry that having them here would hurt a lot. |
Yeah use a generator instead. |
Fair enough, although you only need to do it once and even 1M frames will only generate a 2MB array (I think our ints are 16bit). Alternatively: find the NumPy code that does the slicing. Oliver Beckstein
|
Why can't we use def _get_slice_frame_indices(self, trjslice):
# have some guards here.
return range(trjslice) |
Excellent idea – catch fancy indexing first and then hand over to six.range / xrange.
Oliver Beckstein * orbeckst@gmx.net |
Btw, to make the start = start if start is not None else trajectory.n_frames (It does not handle I think that this is all that is needed. |
Partially, negative stepping requires a new and improved DCD reader IIRC. |
EDIT: I should think before I type.
Thus, there isn't really any big hurdle here. Just the usual write the code and test (and give proper error messages for any reader that cannot do random access yet) – once #314 is solved, all readers can just do this. |
The original report by @richardjgowers asks for trajectory slicing working like list slicing. However, wouldn't it be even better if it worked like numpy array slicing, i.e., including fancy indexing and boolean indexing? The only difference to array slicing is that we wouldn't return a view or copy but still an iterator. |
I think I've implemented boolean slicing... On Wed, 14 Sep 2016, 6:02 p.m. Oliver Beckstein, notifications@github.com
|
@orbeckst yeah so rather than pythonic, we're aiming for numpythonic slicing (like we do with Groups too) So thinking about this wrt #239 it would make sense if the slicing returned a generator with its own file handle, so traj1 = u.trajectory[::2]
traj2 = u.trajectory[::4] Returned generators that were independent of each other, I think currently they won't play nice with each other. |
Separate file handles would be great because then it would be easier to implement analysis that compares a trajectory with itself. For parallel threaded analysis it would also be great (if on can get around the GIL). Oliver Beckstein
|
I would like to handle this error. I have understood what the bug is, but I am not sure where to start from. Please guide me. |
Isn't this fixed already? import MDAnalysis as mda
from MDAnalysisTests.datafiles import PSF, DCD
u = mda.Universe(PSF, DCD)
for ts in u.trajectory[8::-1]:
print(ts.frame) prints just as expected:
Or did I miss something? |
It seems to be fixed by #1081. Sorry @shobhitagarwal1612, you will have to find an other issue to tackle 😃 |
Maybe we should check there's a test somewhere where we go through all sorts of slices and check a numpy array and a reader react the same way |
@jbarnoud
Shouldn't it return the complete list in reverse order according to the Python docs? |
@shobhitagarwal1612 Indeed! This is what I missed! I though I could close an issue for free... We have our culprit here: https://github.com/MDAnalysis/mdanalysis/blob/develop/package/MDAnalysis/coordinates/base.py#L1312 So fixing the issue is a matter of a very little change, but also some tests. |
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
Expected behaviour
Slicing a trajectory should follow all the rules of slicing a Python list
Actual behaviour
Some slices raise errors (with appropriate error messages)
Code to reproduce the behaviour
Currently version of MDAnalysis:
(run
python -c "import MDAnalysis as mda; print(mda.__version__)"
)The text was updated successfully, but these errors were encountered: