NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

phobson merged 1 commit intomatplotlib:masterfromjklymak:mnt-deprecate-yaaa

Jun 5, 2018

Merged

MNT: deprecate patches.YAArrow#11213

phobson merged 1 commit intomatplotlib:masterfromjklymak:mnt-deprecate-yaaa

Jun 5, 2018

Conversation

Copy link

Member

jklymak commentedMay 9, 2018

PR Summary

So far as I can see

matplotlib/lib/matplotlib/patches.py

Line 1275 in4d1ca36

classYAArrow(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 added this to thev3.0 milestone

May 9, 2018

Copy link

Member

ImportanceOfBeingErnest commentedMay 9, 2018

Careful, I don't thinkFancyArrow can be a replacement for that. PossiblyFancyArrowPatch can be, but better check first. If not, better not to state anyalternative 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.

MNT: deprecate patches.YAArrow

3e94e2c

jklymak force-pushed themnt-deprecate-yaaa branch from94eb770 to3e94e2cCompare

May 9, 2018 23:32

Copy link

MemberAuthor

jklymak commentedMay 9, 2018

Ooops, I meantFancyArrowPatch.

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 requested review fromefiring anddstansby

May 10, 2018 00:06

Copy link

Member

ImportanceOfBeingErnest commentedMay 10, 2018

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

Copy link

Contributor

afvincent commentedMay 10, 2018

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 followingcomment )

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

dstansby previously requested changes

May 10, 2018

View reviewed changes

Copy link

Member

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.