-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
One problem I see off the bat is that it would consume an iterator passed
|
In that case, maybe the first approach (catching the exception) would be the safest thing to do? |
@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? |
It looks like the point of that code is to generate a key so things like 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? |
@astrofrog Do you still want an fix in mpl or are you happy with the fix in astropy? |
@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 |
Can you put together that PR please? |
…e returned axes arguments defined __getitem__ but was not iterable.
@tacaswell - I've attached a PR to this issue. |
Is a changelog entry needed? |
I don't think it needs a change log. |
# will fail. | ||
try: | ||
v = tuple(v) | ||
except: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
Travis is now passing \o/ |
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) |
At the very least, it should catch Exception. bare excepts are a bad idea On Wed, Nov 26, 2014 at 6:07 PM, Eric Firing notifications@github.com
|
@efiring - |
@efiring - so you are suggesting removing the call to |
@efiring @WeatherGod - I removed the redundant call to |
Issue with iterability of axes arguments [backport to 1.4.x]
@mdboom Can you back-port this? |
Sure. |
Issue with iterability of axes arguments [backport to 1.4.x]
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 typeastropy.wcs.WCS
. Infigure.py
, the following code:results in an error, specifically when doing
tuple(v)
. The issue is thatiterable(wcs)
returnsTrue
buttuple(wcs)
crashes. As a general issue,iter(v)
working does not guarantee thattuple(v)
will work. One solution is to simply do:I'm not sure if there is a better way to deal with this kind of issue. Just for the record,
tuple(wcs)
gives: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 thatiterable
should not rely just oniter
. Maybe theiterable
function:in
cbook.py
should be using:? (which does raise an error in my case)
cc @mdboom, since it involves the WCS class.