Skip to content

MNT: deprecate patches.YAArrow#11213

Merged
phobson merged 1 commit intomatplotlib:masterfrom
jklymak:mnt-deprecate-yaaa
Jun 5, 2018
Merged

MNT: deprecate patches.YAArrow#11213
phobson merged 1 commit intomatplotlib:masterfrom
jklymak:mnt-deprecate-yaaa

Conversation

@jklymak
Copy link
Copy Markdown
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
Copy Markdown
Member

ImportanceOfBeingErnest commented May 9, 2018

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
Copy Markdown
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 dstansby and efiring May 10, 2018 00:06
@ImportanceOfBeingErnest
Copy link
Copy Markdown
Member

ImportanceOfBeingErnest commented May 10, 2018

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

@afvincent
Copy link
Copy Markdown
Contributor

afvincent commented May 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 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

dstansby commented May 10, 2018

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