Skip to content

py3k fix #2962

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 1 commit into from
Apr 9, 2014
Merged

py3k fix #2962

merged 1 commit into from
Apr 9, 2014

Conversation

ocefpaf
Copy link
Contributor

@ocefpaf ocefpaf commented Apr 8, 2014

Hi,

This is probably not the right way to fix this issue, but I would like to draw attention to this bug when using parasite_axes.py on python3.3.

The legend handler was returning an

AttributeError: 'generator' object has no attribute 'extend'

Thanks for matplotlib!

-Filipe

@@ -263,6 +263,7 @@ def _get_legend_handles(self, legend_handler_map=None):
all_handles = Axes_get_legend_handles(self, legend_handler_map)

for ax in self.parasites:
all_handles = list(all_handles)
Copy link
Member

Choose a reason for hiding this comment

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

This line should go outside of the loop, you only need to convert this to a list once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tacaswell
But the issue is the .extend method (line right below, but still inside the loop) being applied to a generator.

Copy link
Member

Choose a reason for hiding this comment

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

Right and

all_handles = list(all_handles) 

runs through the generator until it is exhausted and make all_handles a list. The loop should then work correctly.

@tacaswell
Copy link
Member

This looks about right to me.

Chasing thing down it actually isn't a py3k issue, if you look at https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/axes/_axes.py#L212 Axes_get_legend_handles really does return a generator. This is a regression that should show up on all versions of python.

@tacaswell tacaswell added this to the v1.4.0 milestone Apr 9, 2014
@tacaswell
Copy link
Member

Could you also add a note to CHANGELOG about fixing this?

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Apr 9, 2014

@tacaswell
Interesting, I was biased to a py3k issue because I just moved to a py3k machine.
I Added a Changelog message and moved the list outside the loop (Just test it here and seems OK).
What do you think?

@tacaswell
Copy link
Member

I like the code, I do not like the text of the changelog. I do not think this is a py3k issue, rather just a bug.

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Apr 9, 2014

You are right! I still had my mind at py3k bug. Just changed that.

@tacaswell
Copy link
Member

No worries, py3k has enough issues it is tempting to blame everything on at ;) My first assumption was that this was a dictionary keys issues that slipped through the cracks.

One last request, could you squash your commits into one?

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Apr 9, 2014

I'm not sure how to do that, and based on what I read I cannot do that when I already pushed to github. Is it OK if we close this and I make a new PR with one commit?

@tacaswell
Copy link
Member

$ git rebase -i upstream/master (assuming the remote that points towards the main repo is called upstream). It should provide a reasonable on-screen instruction from there.

Once you have done the squashing do a git push github --force. In general, this is bad practice, however no one in their right mind should consider a feature branches on a personal repository to be 'released' code so in this case it is OK.

Doing it this way does little harm, keeps the discussion all in one place, and keeps from running up our issue count.

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Apr 9, 2014

@tacaswell
I think I got it right. Thanks a lot for the help! I'm not a git nor python expert and I learned a lot from this PR.

@tacaswell
Copy link
Member

Looks good. Typically we add tests to make sure this does not regress, however the code coverage in mpl_toolkits is very thin/non existent so we will not worry about it in this PR.

Thanks for fixing this issue! Hopefully this is the first of many.

tacaswell added a commit that referenced this pull request Apr 9, 2014
@tacaswell tacaswell merged commit 47d8420 into matplotlib:master Apr 9, 2014
@ocefpaf ocefpaf deleted the axes_grid1_parasite_axes_py3k_fix branch April 9, 2014 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants