Skip to content

Issue with iterability of axes arguments [backport to 1.4.x] #3196

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

Merged
merged 3 commits into from
Dec 3, 2014

Conversation

astrofrog
Copy link
Contributor

I have been trying to use the _as_mpl_axes method to create a custom projection. I am returning an Axes subclass and a dictionary of arguments to pass to the class. One of the arguments is an item of type astropy.wcs.WCS. In figure.py, the following code:

        def fixitems(items):
            #items may have arrays and lists in them, so convert them
            # to tuples for the key
            ret = []
            for k, v in items:
                if iterable(v):
                    v = tuple(v)
                ret.append((k, v))
            return tuple(ret)

results in an error, specifically when doing tuple(v). The issue is that iterable(wcs) returns True but tuple(wcs) crashes. As a general issue, iter(v) working does not guarantee that tuple(v) will work. One solution is to simply do:

if iterable(v):
    try:
        v = tuple(v)
    except:
        pass

I'm not sure if there is a better way to deal with this kind of issue. Just for the record, tuple(wcs) gives:

ValueError: Cannot downsample a WCS with indexing.  Use wcs.sub or wcs.dropaxis if you want to remove axes.

To some extent, the issue here is that while WCS has a __getitem__ method, it is not strictly iterable. So maybe the issue is actually that iterable should not rely just on iter. Maybe the iterable function:


def iterable(obj):
    'return true if *obj* is iterable'
    try:
        iter(obj)
    except TypeError:
        return False
    return True

in cbook.py should be using:

next(iter(wcs))

? (which does raise an error in my case)

cc @mdboom, since it involves the WCS class.

@WeatherGod
Copy link
Member

One problem I see off the bat is that it would consume an iterator passed
to it, rendering it useless after calling. This would be a problem in py3k.
On Jul 7, 2014 2:54 PM, "Thomas Robitaille" notifications@github.com
wrote:

I have been trying to use the _as_mpl_axes method to create a custom
projection. I am returning an Axes subclass and a dictionary of arguments
to pass to the class. One of the arguments is an item of type
astropy.wcs.WCS. In figure.py, the following code:

    def fixitems(items):
        #items may have arrays and lists in them, so convert them
        # to tuples for the key
        ret = []
        for k, v in items:
            if iterable(v):
                v = tuple(v)
            ret.append((k, v))
        return tuple(ret)

results in an error, specifically when doing tuple(v). The issue is that
iterable(wcs) returns True but tuple(wcs) crashes. As a general issue,
iter(v) working does not guarantee that tuple(v) will work. One solution
is to simply do:

if iterable(v):
try:
v = tuple(v)
except:
pass

I'm not sure if there is a better way to deal with this kind of issue.
Just for the record, tuple(wcs) gives:

ValueError: Cannot downsample a WCS with indexing. Use wcs.sub or wcs.dropaxis if you want to remove axes.

To some extent, the issue here is that while WCS has a getitem
method, it is not strictly iterable. So maybe the issue is actually that
iterable should not rely just on iter. Maybe the iterable function:

def iterable(obj):
'return true if obj is iterable'
try:
iter(obj)
except TypeError:
return False
return True

in cbook.py should be using:

next(iter(wcs))

? (which does raise an error in my case)

cc @mdboom https://github.com/mdboom, since it involves the WCS class.


Reply to this email directly or view it on GitHub
#3196.

@astrofrog
Copy link
Contributor Author

In that case, maybe the first approach (catching the exception) would be the safest thing to do?

@tacaswell tacaswell added this to the v1.4.x milestone Jul 12, 2014
@astrofrog
Copy link
Contributor Author

@mdboom @WeatherGod @tacaswell - any thoughts on this bug? I got bit again today too so just wanted to check whether I should go ahead and open a PR implementing the exception catching solution?

@tacaswell
Copy link
Member

It looks like the point of that code is to generate a key so things like
plt.subplot(1, 1, 1) can tell if you have already added that axes and return back to you the existing axes instead of making a new one. (My view is that this is a feature that is more trouble than it is worth, but that is neither here nor there for the issue at hand).

A PR with the exception catching is ugly, but fine. Ideally, there should be a more reliable way to make a hash out of an arbitrary object. Maybe the function should first try to hash the object before turning it into a tuple?

@tacaswell
Copy link
Member

@astrofrog Do you still want an fix in mpl or are you happy with the fix in astropy?

@astrofrog
Copy link
Contributor Author

@tacaswell - I still think that we should put a proper fix into Matplotlib since what I did in astropy is just a workaround. I'm happy to do a PR but wanted to wait first to see if there were other suggestions on implementation. I can see why using next might be problematic.

@tacaswell
Copy link
Member

Can you put together that PR please?

…e returned axes arguments defined __getitem__ but was not iterable.
@astrofrog
Copy link
Contributor Author

@tacaswell - I've attached a PR to this issue.

@astrofrog
Copy link
Contributor Author

Is a changelog entry needed?

@tacaswell
Copy link
Member

I don't think it needs a change log.

# will fail.
try:
v = tuple(v)
except:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you catch a specific exception or is that not practical here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can - it completely depends on what __getitem__ raises. It won't necessarily be a KeyError.

@astrofrog
Copy link
Contributor Author

Travis is now passing \o/

@efiring
Copy link
Member

efiring commented Nov 26, 2014

It's a bit ugly; would the following do the job?

        def fixitems(items):
            # items may have arrays and lists in them, so convert them
            # to tuples for the key
            ret = []
            for k, v in items:
                try:
                    v = tuple(v)
                except TypeError:
                    pass
                ret.append((k, v))
            return tuple(ret)

@tacaswell tacaswell changed the title Issue with iterability of axes arguments Issue with iterability of axes arguments [backport to 1.4.x] Nov 26, 2014
@WeatherGod
Copy link
Member

At the very least, it should catch Exception. bare excepts are a bad idea
(it even catches ctrl-c's!)

On Wed, Nov 26, 2014 at 6:07 PM, Eric Firing notifications@github.com
wrote:

It's a bit ugly; would the following do the job?

    def fixitems(items):
        # items may have arrays and lists in them, so convert them
        # to tuples for the key
        ret = []
        for k, v in items:
            try:
                v = tuple(v)
            except TypeError:
                pass
            ret.append((k, v))
        return tuple(ret)


Reply to this email directly or view it on GitHub
#3196 (comment)
.

@astrofrog
Copy link
Contributor Author

@efiring - TypeError would not be ok - for example in Astropy, the WCS class' __getitem__ returns a ValueError if one passes a single integer to __getitem__. I'll add Exception though.

@astrofrog
Copy link
Contributor Author

@efiring - so you are suggesting removing the call to iterable? I'd be fine with that and agree it's cleaner.

@astrofrog
Copy link
Contributor Author

@efiring @WeatherGod - I removed the redundant call to iterable and am now catching only Exception.

mdboom added a commit that referenced this pull request Dec 3, 2014
Issue with iterability of axes arguments [backport to 1.4.x]
@mdboom mdboom merged commit 14475ab into matplotlib:master Dec 3, 2014
@tacaswell
Copy link
Member

@mdboom Can you back-port this?

@mdboom
Copy link
Member

mdboom commented Dec 3, 2014

Sure.

mdboom added a commit to mdboom/matplotlib that referenced this pull request Dec 3, 2014
Issue with iterability of axes arguments [backport to 1.4.x]
tacaswell added a commit that referenced this pull request Dec 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants