Units: Quantity always thinks it is iterable

[embray]

Don't have time to look at this today, but probably can tackle over the weekend.

>>> astropy.utils.isiterable([15.,17.]*u.yr)
True
>>> astropy.utils.isiterable(15.*u.yr)
True

[adrn]

It looks like the root of the problem here is that in isiterable(), it has this block:

try:
    iter(obj)
    return True
except TypeError:
    return False

But because Quantity has __getitem__() defined, it can always return an iterator.

How about changing it to:

try:
    iter(obj).next()
    return True
except TypeError:
    return False

I have a branch ready to go with this in a PR if it seems ok...tests still pass.

[mdboom]

Seems reasonable.

[taldcroft]

Is there any case where iterating over an iterable (via next()) might have side effects? It's easy enough to contrive such an example, but I don't know if there are realistic cases.

[adrn]

@taldcroft Hm, I guess it could be an issue for file-like objects?

[taldcroft]

For file-like objects the test of isinstance(obj, collections.Iterable) passes so the iter(obj).next() never gets hit.

Just getting nitpicky, an iterable object can have zero length (like []), in which case the next() call will produce a StopIterationError:

>>> iter([]).next()
ERROR: StopIteration [IPython.core.interactiveshell]

So to me the next() addition seems like not the best strategy here, though I don't have a better alternative...

[adrn]

Good point, but that too would get caught by isinstance(obj, collections.Iterable), right?

[alexrudy]

This issue probably runs a bit deeper than astropy's isiterable() test.

I've hit it in my own code (in which I often use things like collections.Sized and collections.Iterable from python's collections module).

I have the vague idea of hiding the irrelevant attribute methods behind __getattr__ and __hasattr__ functions which would only expose functions like __len__ and other collection methods when that is appropriate (based on .isscalar). Probably smarter would be to separate Quantity in the scalar and non-scalar case, although that drags in the whole question from #929. If Quantity is going to be useful data type for me, then duck typing and ABC typing need to work without manually checking for .isscalar.

By the way, collections.Iterable checks for the presence of an __iter__ method and collections.Iterator checks for __next__, both of which duck-type check correctly.

[embray]

I think that if we are successful in making Quantity an ndarray subtype then that should resolve this issue. isiterable seems to work for "array scalars":

In [9]: isiterable(np.array(1))
Out[9]: False

[alexrudy]

@iguananaut I suspected this might well be part of the solution, however, it is still a little unsatisfying. I suspect that the behavior of "array scalars" are well defined upstream. Although they pass the astropy isiterable test, ArrayScalars don't play nice with python duck-typing in other situations. Really, if we all wrote our python "correctly", we wouldn't need the special isiterable method, and we'd be able to tell just by checking for the presence of the __iter__ method, like built-in python does in collections etc.

Its unfortunate that Numpy breaks this with scalar arrays, and an argument which suggests we might not want to move entirely to scalar arrays for Quantity. (Although there are plenty of great arguments in the other direction)

Do you know if the Numpy crowd has ever considered fixing this?

[embray]

I agree that Numpy scalar arrays are broken in this sense. I think I'm just okay with it because everyone who's used Numpy has just learned that it's something we have to accept. I'd be all for finding a way to fix this from the Numpy end though.

[eteq]

Yeah, the borken Numpy scalar arrays were the reason for our own isiterable in the first place.

Two alternatives for @adrn's approach:

1. implement an __iter__ method in Quantity that checks if self.isscalar, and if so issues a TypeError, and if not, does whatever the current behavior is. I think that will fix this
2. Just short-circuit the problem by adding this at the top if isiterable:

if isinstance(obj, Qunatity):
    return not obj.isscalar

[embray]

I like alternative 1️⃣ I'll try it out and attach a PR if it works.

[embray]

Well, turns out that doesn't work because as soon as you define __iter__ the object becomes an instance of collections.Iterable. That said, defining __iter__ is still a good idea so that we can raise a TypeError as soon as one tries iter(q) on a scalar quantity. So I will use a combination of 1️⃣ and 2️⃣.

[embray]

Nope: That doesn't quite do it either. Putting 'import Quantity' anywhere in utils.misc (even inside the function call) leads to a nasty import loop. This isn't too surprising actually, and in general stuff in utils shouldn't depend on anything else in Astropy--it should be a one way street. So finally I just removed the isinstance(obj, collections.Iterable) and let it fall back on checking if iter(obj) works, which can be slower but generally won't be.

[embray]

Compromise: I changed it to if not isinstance(obj, collections.Iterable). That will work fine for any negatives. But then it checks iter(obj) to catch any false positives.

[eteq]

whoops, looks like this needs a rebase. Other than that, looks good to me!

[embray]

Rebased.

[eteq]

it appears this line is necessary - raises is used below.

[embray]

I noticed that too, just forgot to delete it.

[eteq]

Looks like it needs another rebase (changes.rst?) or manual merge, but more importantly, the tests are failing. Looks like this changed some behavior that table and parts of fits depend on - any idea what's going on here, @iguananaut ?

[embray]

All those failures are surprising...I'd think I'd have run the tests before pushing but maybe I didn't?

[embray]

The problem here is a bit tricky. ABCs can implement a magic classmethod call __subclasshook__ that it uses to modify the behavior of issubclass (and by extension isinstance). For example collections.Iterable defines a __subclasshook__ which returns True on any class that implements an __iter__ method. That way anything with an __iter__ is automatically a subclass of collections.Iterable even if it doesn't explicitly inherit from this.

According to http://docs.python.org/2/library/collections.html, a class that implements the __getitem__ and __len__ methods can be used as a collections.Sequence implementation (which is also automatically a subclass of Iterable). I assumed (wrongly) that Sequence would also define a __subclasshook__ for this but apparently it does not. This fact is not at all documented, and all I could find were some mailinglist discussions about how it's too hard to determine by introspection that a class with __getitem__ and __len__ will truly behave correctly as a sequence. This fact is the source of the test failures.

I still think this is a bug in Python--both in the documentation and the behavior, because even if a class is not properly identifiable as a sequence, calling iter() on it will still succeed at least up to a point. It's really inconsistent on Python's part that you can call iter() on something that the ABC system can't recognized as Iterable. So until this is fixed I'm removing the check for collections.Iterable completely until it can be relied on to behave more consistently.

I also went ahead and registered a few classes as implementing existing collection types where appropriate.

[embray]

Indicentally, registering the mentioned classes as Sequences fixed all the tests without having to make any changes to isiterable. But I'm still changing isiterable anyways since the behavior of the Sequence ABC is too unreliable here.

[eteq]

Wow that's surprisingly subtle. I wonder if you should report it to the python core?

Anyway, it's working as expected for us now, so I'll go ahead and merge (I'll just manually fix up CHANGES.rst because that's all that's conflicting).

[eteq]

Weird that this is "closed" instead of "merged". Ah well, nevertheless, it's in.

[embray]

And as such this got missed by my backport script, so good thing I double-checked. Usually I've seen GitHub be pretty good about attaching a manual merge to a pull request but as you said that didn't happen this time :/