Skip to content

FIX: remove repeated label legend logic #10064

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
Dec 21, 2017

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Dec 20, 2017

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 API: consistency labels Dec 20, 2017
@jklymak
Copy link
Member Author

jklymak commented Dec 20, 2017

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.

@@ -0,0 +1,20 @@
`Figure.legend` no longer checks for repeated lines to ignore
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 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
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
Member

Choose a reason for hiding this comment

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

no worries!

Copy link
Member

@tacaswell tacaswell left a comment

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
Copy link
Member Author

jklymak commented Dec 21, 2017

... docs moved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: changes API: consistency 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