8000 Soft-deprecate mutation_aspect=None. by anntzer · Pull Request #17750 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Soft-deprecate mutation_aspect=None. #17750

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 28, 2020
Merged

Soft-deprecate mutation_aspect=None. #17750

merged 1 commit into from
Jun 28, 2020

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented Jun 24, 2020

mutation_aspect=1 is a synonym that's just simpler and consistent with
other scalar values.

Not really worth a proper deprecation; just changing the defaults in the
signatures and normalizing None to 1 in the getter.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@QuLogic
Copy link
Member
QuLogic commented Jun 25, 2020

CI doesn't think it's so simple...

@anntzer
Copy link
Contributor Author
anntzer commented Jun 25, 2020 •
edited
Loading

Looks like this exposed another bug, namely annotate("foo", (.5, .5), xytext=(.4, .4), arrowprops={"arrowstyle": "->", "mutation_scale": 2}) likely never ever worked; one just needs to get rid of a spurious zip call.
Edit: and another one, transmute() was previously called with the wrong arguments (linewidth, mutation_scale instead of mutation_scale, linewidth) -- fixed, but I wonder whether we should just deprecate mutation_aspect for arrowstyle, given that it doesn't really make sense there anyways...

@QuLogic QuLogic added this to the v3.4.0 milestone Jun 26, 2020
tacaswell
tacaswell approved these changes Jun 26, 2020
View reviewed changes
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.

Anyone can merge on green.

Might be worth adding a and != 1 to the if condition to avoid doing some non-productive work?

@timhoffm
Copy link
Member

Which if?

@anntzer
Copy link
Contributor Author
anntzer commented Jun 26, 2020

I wouldn't bother (as usual, unless one shows a real-world case where this is the bottleneck), the cost should typically be negligible compare to the actual transmutation...

@anntzer
Soft-deprecate mutation_aspect=None.
a8c2b01 
mutation_aspect=1 is a synonym that's just simpler and consistent with
other scalar values.

Not really worth a proper deprecation; just changing the defaults in the
signatures and normalizing None to 1 in the getter.

Also fixes 2 bugs exposed on this code path.
@tacaswell
Copy link
Member

@timhoffm The one at 2889 the determines if we go into "actually do the scaling" code path.

@tacaswell tacaswell merged commit b35a5e9 into matplotlib:master Jun 28, 2020
@anntzer anntzer deleted the mutation_aspect branch June 28, 2020 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@anntzer @QuLogic @timhoffm @tacaswell
0