Skip to content

Conversation

@MarcoGorelli
Copy link
Member

@MarcoGorelli MarcoGorelli commented Apr 4, 2021

I spent several hours on this, and I don't think there's a simple solution at the moment (though I'd love to be proved wrong!)

Background:

in Nov 2017, #18222 was opened, in which it was noted that marker info isn't preserved in legends. This is because we were getting legend handles using leg.legendHandles:

handles = leg.legendHandles

Due to an open issue in matplotlib, leg.legendHandles doesn't preserve marker info. So, in #27808, the code was updated to use ax.get_legend_handles_labels:

handle, _ = ax.get_legend_handles_labels()

This preserves marker info but has the problem that it may contain additional objects which aren't visible in the legend. The result of this is the permutations observed in #39522 .

I think that having wrong colours in the legend, as in #39522 and #40044, is far worse than the legend not preserving marker info. Solving everything together doesn't currently seem doable, at least not until matplotlib/matplotlib#11357 is solved in matplotlib.

So my suggestion is to revert some of the changes in #27808, so that at least the colors are right, and when the matplotlib issue is addressed, then the marker info can be preserved.


#39522 on master:

image

on this branch:

image


#40044 on master:

image

on this branch:

image

@MarcoGorelli MarcoGorelli added the Visualization plotting label Apr 4, 2021
@MarcoGorelli MarcoGorelli marked this pull request as draft April 4, 2021 18:54
@MarcoGorelli MarcoGorelli marked this pull request as ready for review April 5, 2021 12:40
@jreback
Copy link
Contributor

jreback commented Apr 5, 2021

is #39522 closed? (or partial)

@jreback jreback added this to the 1.3 milestone Apr 5, 2021
@@ -0,0 +1,45 @@
import pytest

Copy link
Contributor

Choose a reason for hiding this comment

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

can you move other legend tests here? (if we have them)

@MarcoGorelli
Copy link
Member Author

@jreback it's only a partial fix of #39522 , as the marker info isn't preserved

can you move other legend tests here? (if we have them)

Sure, though for ease of review, should I do that in a follow-up PR?

@jreback
Copy link
Contributor

jreback commented Apr 5, 2021

Sure, though for ease of review, should I do that in a follow-up PR?

either way

@MarcoGorelli
Copy link
Member Author

OK, have moved those tests over in this pr @jreback

@jreback jreback merged commit d625880 into pandas-dev:master Apr 9, 2021
@jreback
Copy link
Contributor

jreback commented Apr 9, 2021

thanks @MarcoGorelli very nice!

@MarcoGorelli MarcoGorelli deleted the fix-legend-yerr branch April 9, 2021 07:36
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 participants