FrameIteratorIndices doesn't rewind whilst FrameIteratorSliced does

[IAlibay]

Related to #3415

Expected behavior

See #3415 (comment)

Iterating through FrameIteratorSliced should leave u.trajectory at the same point as if I iterated through FrameIteratorIndices.

Actual behavior

The former rewinds whilst the latter doesn't.

I might be missing a pythonic reason (which I'd be happy to accept), just bringing this up because of discussions on #3415

Code to reproduce the behavior

import MDAnalysis as mda
from MDAnalysisTests.datafiles import TPR, XTC

u = mda.Universe(TPR, XTC)

iterator = u.trajectory[[0,1,2]]

for ts in iterator:
    pass

print(u.trajectory.ts)

Ouput: < Timestep 2 with unit cell dimensions [79.98359 79.98359 79.98358 60. 60. 90. ] >

Whilst:

import MDAnalysis as mda
from MDAnalysisTests.datafiles import TPR, XTC

u = mda.Universe(TPR, XTC)

for ts in u.trajectory[:3]
    pass

print(u.trajectory.ts)

Ouput: < Timestep 0 with unit cell dimensions [80.017006 80.017006 80.017006 60. 60. 90. ] >

Current version of MDAnalysis

• Which version are you using? (run python -c "import MDAnalysis as mda; print(mda.__version__)") develop
• Which version of Python (python -V)? 3.9

[IAlibay]

Looking into it more, looks like a possibly intention decision by @jbarnoud ?

[orbeckst]

I looked through PR #1934 and although there are some interesting comments like #1934 (comment) (why FrameIterators are like range() and not really iterators) I couldn't see a real reason why FrameIteratorSliced rewinds in __iter__

mdanalysis/package/MDAnalysis/coordinates/base.py

Lines 992 to 995 in 3985021

	def __iter__(self):
	for i in range(self.start, self.stop, self.step):
	yield self.trajectory[i]
	self.trajectory.rewind()

while FrameIteratorIndices does not

mdanalysis/package/MDAnalysis/coordinates/base.py

Lines 1112 to 1115 in 3985021

	def __iter__(self):
	for frame in self.frames:
	yield self.trajectory._read_frame_with_aux(frame)

Given that FrameIteratorAll just defers to the reader's behavior (which rewinds) it looks to me that FrameIteratorIndices should do the same.

I can't think of a use case where this should be handled differently, especially as it breaks user expectations (and is not documented either).

[jbarnoud]

From what I can see, there is no reason for the inconsistency. All the frame iterators should rewind. On 8 Sep 2021 02:41, Oliver Beckstein ***@***.***> wrote: I looked through PR #1934 and although there are some interesting comments like #1934 (comment) (why FrameIterators are like range() and not really iterators) I couldn't see a real reason why FrameIteratorSliced rewinds in __iter__ https://github.com/MDAnalysis/mdanalysis/blob/39850215eae5a3e2c9d28a24dbc670646e305f79/package/MDAnalysis/coordinates/base.py#L992-L995 while FrameIteratorIndices does not https://github.com/MDAnalysis/mdanalysis/blob/39850215eae5a3e2c9d28a24dbc670646e305f79/package/MDAnalysis/coordinates/base.py#L1112-L1115 Given that FrameIteratorAll just defers to the reader's behavior (which rewinds) it looks to me that FrameIteratorIndices should do the same. I can't think of a use case where this should be handled differently, especially as it breaks user expectations (and is not documented either). —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications on the go with GitHub Mobile for iOS or Android.

[IAlibay]

Looks like the consensus here is to just add the rewind. Do we need to warn or can we treat this as a bug and fix in 2.1.0?

[orbeckst]

I think we can treat it as a bug fix.

[orbeckst]

As elsewhere #3423 (comment) I'd like to label this issue a defect because the behavior is not documented and wasn't intended as such. A fix would fix the API to be consistent with the rest of the readers.

[orbeckst]

If we get this (and #3423 ) done then we can also finish up PR #3415 .

[IAlibay]

If we get this (and #3423 ) done then we can also finish up PR #3415 .

Yeah I'll try to have a butcher this weekend maybe - got to get CI back to green first.

[megosato]

Hello!
I was hoping to work on this issue, but wanted to confirm a few things.

My current understanding is:
FrameIteratorIndices should rewind to Time Step 0 similar to FrameIteratorSliced and FrameIteratorAll

Questions:
If rather than making the iterator iterator = u.trajectory[[0,1,2]] we do iterator = u.trajectory[[2,3,4]], should it still rewind to timestep 0? And likewise for FrameIteratorSliced?

I'm also wondering logistically if/how bugs are handled differently from defects from a programming and documentation perspective? (per #3416 (comment))

Thank you for your help!

[orbeckst]

My current understanding is: FrameIteratorIndices should rewind to Time Step 0 similar to FrameIteratorSliced and FrameIteratorAll

Yes.

If rather than making the iterator iterator = u.trajectory[[0,1,2]] we do iterator = u.trajectory[[2,3,4]], should it still rewind to timestep 0? And likewise for FrameIteratorSliced?

Good question. I'd say yes, for consistency, because that's what FrameIteratorSliced is already doing. (The alternative would be to rewind to the first frame index but that seems less consistent with what we have at the moment.)

I'm also wondering logistically if/how bugs are handled differently from defects from a programming and documentation perspective? (per #3416 (comment))

I use bugs and defects synonymously. What difference do you perceive?

In this specific case the question was if the change will be an API change or a bug fix (= defect fix). Because we use semantic versioning this has wide-ranging implication. Under SemVer, we can only make backwards-incompatible API changes when we increase the major release number (e.g., the next one would be 3.0.0). Bug-fixes can go in a patch release, e.g., 2.1.1 or in the next minor release 2.2.0. The consensus seemed to be that this is not an intentional API change but we are fixing undocumented behavior. Thus, a PR can actually be merged right away and does not have to wait for 3.0.0.