Skip to content

FIX: remove repeated label legend logic#10064

Merged
dstansby merged 1 commit into
matplotlib:masterfrom
jklymak:fix-legend-logic
Dec 21, 2017
Merged

FIX: remove repeated label legend logic#10064
dstansby merged 1 commit into
matplotlib:masterfrom
jklymak:fix-legend-logic

Conversation

@jklymak

@jklymak jklymak commented Dec 20, 2017

Copy link
Copy Markdown
Member

PR Summary

Fixes #10030, #10053, #10056.

Pre #9324, there was logic in Figure.legend() to not include duplicate labels in the legend if the linecolors or marker colors were the same. That logic was buggy, and didn't include all possible line properties, but presumably users of Figure.legend() worked around this.

In #9324, I homogenized the logic between Figure.legend() and Axes.legend(). I still think that re-factoring was a good thing to do, but I missed that Axes.legend() didn't have the no-duplicate logic in it. That exposed the bugs noted above.

This PR, goes the other way of making Figure.legend() the same as pre-9324 Axes.legend() and not removing the duplicate legend entries.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak jklymak added this to the v2.1.2 milestone Dec 20, 2017
@jklymak jklymak added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. API: changes Changes to the public API, typically requiring deprecation. API: consistency Consistency of the matplotlib API, including naming, behavior, defaults, … labels Dec 20, 2017
@jklymak

jklymak commented Dec 20, 2017

Copy link
Copy Markdown
Member Author

This of course failed a test in test_figure that checked if the repeated label was ignored. I replaced the label name by "_y" instead of "y" to keep the images the same.

Comment thread doc/api/api_changes/2017-12-20-JMK.rst Outdated
@@ -0,0 +1,20 @@
`Figure.legend` no longer checks for repeated lines to ignore

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you put this directly in api_changes.rst. The reason we put them in individual files is to avoid interminable rebases when the docs overlap, but this should be the only 2.1.2 related API change so that is not a risk ;)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No problem. But I’ll be away from machine for a while.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no worries!

@tacaswell tacaswell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Modulo moving the API docs

@jklymak

jklymak commented Dec 21, 2017

Copy link
Copy Markdown
Member Author

... docs moved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API: changes Changes to the public API, typically requiring deprecation. API: consistency Consistency of the matplotlib API, including naming, behavior, defaults, … Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants