Skip to content

MNT: deprecate patches.YAArrow #11213

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 5, 2018
Merged

MNT: deprecate patches.YAArrow #11213

merged 1 commit into from
Jun 5, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented May 9, 2018

PR Summary

So far as I can see

matplotlib/lib/matplotlib/patches.py

Line 1275 in 4d1ca36

class YAArrow(Patch):

is not ever called or documented (except in the API). Maybe there is a huge fan base out there, but...

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

@jklymak jklymak added this to the v3.0 milestone May 9, 2018
@ImportanceOfBeingErnest
Copy link
Member

Careful, I don't think FancyArrow can be a replacement for that. Possibly FancyArrowPatch can be, but better check first. If not, better not to state any alternative at all than a wrong one. The point being that FancyArrow seems to live completely in data space (or whatever coordinate system you want) while YAArrow has a transform associated with it.

@jklymak jklymak force-pushed the mnt-deprecate-yaaa branch from 94eb770 to 3e94e2c Compare May 9, 2018 23:32
@jklymak
Copy link
Member Author

jklymak commented May 9, 2018

Ooops, I meant FancyArrowPatch.

I don't know that its a direct replacement, and I don't think stating an alternative necessarily means it is. It just means thats where the user can go to get the same functionality...

@afvincent afvincent requested review from efiring and dstansby May 10, 2018 00:06
@ImportanceOfBeingErnest
Copy link
Member

Relevant: https://github.com/matplotlib/matplotlib/wiki/MEP-29-(arrows) (Not to have this reference getting lost.)

@afvincent
Copy link
Contributor

Pinging @dstansby who recently did some work about an overhaul about arrow overhaul if I remember correctly
Pinging @efiring who did some work on removing YAArrow usage from the codebase (at least in 2015, see for example #4178 and the following comment )

NB: it is more asking for opinions rather than “requesting review” :).

dstansby
dstansby previously requested changes May 10, 2018
View reviewed changes
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

I do not think this should be merged until we have come to an overall decision about which arrow functions we want to keep and which want to go.

I haven't worked on the MEP for a while, but I think a good next step would be writing a tutorial that demonstrates plotting arrows (in display space and data space). I will try and do this in the next couple of weeks.

@dstansby dstansby dismissed their stale review May 10, 2018 14:12

I'm in the process of writing an arrow tutorial, and have come to the conclusion that YAArrow isn't needed

@jklymak
Copy link
Member Author

jklymak commented May 10, 2018

#11219 addresses this via documentation. I’ll close this though still skeptical this is needed.

@jklymak jklymak closed this May 10, 2018
@dstansby
Copy link
Member

Sorry if I confused you @jklymak, I think YAArrow should be deprecated.

@jklymak jklymak reopened this May 10, 2018
@phobson phobson merged commit ced5719 into matplotlib:master Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phobson phobson phobson approved these changes

@dstansby dstansby dstansby approved these changes

@efiring efiring Awaiting requested review from efiring

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

Successfully merging this pull request may close these issues.
5 participants
@jklymak @ImportanceOfBeingErnest @afvincent @dstansby @phobson