Inconsistent Collection() iteration

[geowurster]

Iteration in fiona.Collection() is not "pinned" until the __iter__() method is called. Opening a file and calling next() on the file produces the first feature, but entering a for loop inside the same context manager produces the first feature again on the first iteration. Doing the reverse only produces each feature once.

/ cc @cjthomas730

Examples

Open a file, call next() a couple times, and then loop over the same collection:

print("Individual")
with fio.open('tests/data/coutwildrnp.shp') as src:
    print(next(src)['id'])
    print(next(src)['id'])
    print("Loop")
    for idx, feat in enumerate(src):
        print(feat['id'])
        if idx >= 2:
            break
Individual
0
1
Loop
0
1
2

Loop over the collection a few times and then call next() outside the loop:

print("Loop + individual")
with fio.open('tests/data/coutwildrnp.shp') as src:
    for idx, feat in enumerate(src):
        print(feat['id'])
        print(next(src)['id'])
        if idx > 3:
            break
    print(next(src)['id'])
    print(next(src)['id'])
Loop + individual
0
1
2
3
4
5
6
7
8
9
10
11

Python iterator:

print("Python iterator")
r = iter(range(5))
print(next(r))
print(next(r))
for i in r:
    print(i)
Python iterator
0
1
2
3
4
5

[sgillies]

Good catch @geowurster. In this situation our feature collections should behave just like any other sequence iterator.

>>> itr = iter(range(10))
>>> next(itr)
0
>>> next(itr)
1
>>> list(itr)
[2, 3, 4, 5, 6, 7, 8, 9]

[geowurster]

@sgillies Taking a shot at this one right now.

[geowurster]

@sgillies Found another inconsistency. The documentation specifically says that seeking to the beginning of the file is not supported, however it is technically possible due to how the iteration methods set the internal state. There are some tests suggesting this behavior is intentional, but it goes against Python's iteration protocol. I believe the broken tests in PR #305 exist to to verify the behavior below remains unchanged, which also suggests that changing the behavior will break stuff for some users, so maybe we want to fix this in 1.7?

with fio.open('tests/data/coutwildrnp.shp') as src:
    for feat in src:
        print(feat['id'])
        break
    for fid, feat in src.items():
        print(fid)
        break
    for fid in src.keys():
        print(fid)
        break
    for feat in src:
        print(fid)
        break

0
0
0
0

Assuming we want to change this, we could set up a guard method that checks to see if Collection.iterator is set

Something like:

def _guard_iterator(iterator):
    assert self.iterator is None, "Cannot change iteration type mid-stream."
    self.iterator = iterator
    return self.iterator

def filter(*args, *kwds):
    # Set up Iterator()
    return self._guard_iterator(Iterator())

Another option would be to swap iterators on the fly, which isn't very Pythonic, and would require another method to reset iteration to the default state. I didn't look deep enough to be sure this is possible, but maybe something like:

def _guard_iterator(iterator):
    if self.iterator:
        # Add a method to the Iterator() classes to copy the current position of
        # self.iterator to the new incoming iterator
        iterator.shift(self.iterator.start, self.iterator.stop, self.iterator.step)
    self.iterator = iterator
    return self.iterator

Another option is to have _guard_iterator() and implement a seek() method.

I think my vote is raising an exception if two different iteration methods are called. Letting iterators seek but not directly supporting seek() is obtuse, feels pretty loophole-y, and doesn't match the iterator protocol. Anyone that needs 2 or more in 1 read session can just iterate over the features and extract what they need. Adding a seek() method or swappable iterators down the road would be easier than having to remove them.

[sgillies]

@geowurster that seems right to me. Let's not get users hooked on seeking via anything other than an explicit seek().

In your example above using .items() and .keys()... that's exactly the behavior I'm looking for. It's consistent with iterators over a dict's keys and items.

>>> d = {a: a for a in range(10)}
>>> keys = d.iterkeys()
>>> items = d.iteritems()
>>> next(keys)
0
>>> next(keys)
1
>>> next(items)
(0, 0)

filter()/values(), items(), and keys() are separate iterators will separate state. Users can't expect to interleave them with a shared state.

[sgillies]

@geowurster how about we tag this one for 1.7 – your PR is based on the master branch and since we made the ogrext1/ogrext2 change it's going to be harder to merge fixes from maint-1.6 into master.

[geowurster]

@sgillies Yeah that's fine with me. I'll wait until after we release 1.6.3 and merge master into my branch.

[geowurster]

@sgillies Revisiting this one.

I just tried the _guard_iterator() method mentioned above where we just raise an exception if multiple iteration methods are called and 68 tests failed
so it looks like we should probably be approaching this as a breaking change,
especially since this behavior has likely crept into some workflows.

The problem

I think the primary problem is that we don't enforce the "one iteration per sesson"
rule, and the secondary problem is that we are adopting iteration behavior
from two different objects (file + dict), without fully Fiona-izing their
inherent behaviors.

builtins vs. Fiona Behaviors

With a file we can open it, iterate over the lines, and then seek() back
to a specific position, and iterate over the lines again. With a dictionary
we can iterate over the dict itself or keys|values|items(), but these
iteration methods give us a functionally "detached" iterator that always
gives us an iterator across the entire dict. The contents of the dict are
entirely in-memory so we don't really have a cursor or need to seek().

For Fiona we want both but we don't exactly keep track of the cursor, and we
don't handle the situation where we partially iterate over a Collection() and
then switch iteration methods. Additionally, our "detached" iteration methods
are fully embedded in our file-like object.

---

Solutions

In the end I think the primary question we need to answer is if we want to
allow seeking within a read session.

No Seeking

The most straightforward solution like we outlined earlier with
_guard_iteration(). Only one iteration per read. Whichever iteration method
is called first is used for the entire session. Any subsequent calls to
iteration methods should raise an exception. Calling next() before
__iter__ should should probably kick off __iter__.

Seeking

Is there a reason why an explicit seek() has not yet been written for read
sessions? There are a ton of extra considerations to address to support
writing + updating, but we already do it in ogrext.Iterator() with
OGR_L_SetNextByIndex().

If we enable seeking we could let __iter__|__next__ provide the file-like
interface while keys|items|filter() provide the "detached" dict-like
iteration.

I'm guessing we would have to figure out how to have multiple pointers to the
underlying OGR layer handle - could we just get a new handle any time we call a
detached iteration method in a reasonable amount of time through
OGR_DS_GetLayer()? I have no idea if any formats only support one handle per
layer per datasource.

[sgillies]

@geowurster I've carved out time to get back to Fiona issues this week. I think that seek/tell/next can work together fine for Fiona. We don't have quite the same concerns as Python's file objects do. I still don't have a good grip on the use cases for this. Would you like to do something like iterate over the last N records in a file?

with fiona.open('example.shp') as src:
    src.seek(-N, fiona.SEEK_END)
    for feat in src:
        print(feat)

# Printed:
# 3 features

[sgillies]

And while we're talking about consistency, could we agree that if we implemented seek() the following would hold: src[i] == (src.seek(i); next(src))?