Skip to content

MNT: deprecate arrow #22382

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

Closed
wants to merge 1 commit into from
Closed

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Feb 3, 2022

PR Summary

Closes #20387

ax.arrow is fundamentally broken, and only works for certain types of axes; users should use annotate instead. Not deprecating FancyArrow, but maybe that should happen too.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@jklymak jklymak force-pushed the mnt-deprecate-arrow branch 2 times, most recently from 47dcd04 to 348d1d0 Compare February 3, 2022 10:37
@jklymak jklymak marked this pull request as ready for review February 3, 2022 12:58
@jklymak jklymak added this to the v3.6.0 milestone Feb 3, 2022
@story645
Copy link
Member

story645 commented Feb 3, 2022

I can't remember where this was discussed but I always thought that a problem here is that arrow is explicitly for when the data should be visualized as arrows; annotation doesn't make sense because folks aren't trying to annotate anything. I also forgot why quivers don't work here and this was maybe as part of a request for a vector like thing.

But I do agree on maybe consolidating the underlying primitive/patch to just one type if possible.

@timhoffm
Copy link
Member

timhoffm commented Feb 3, 2022

The fundamental problem with arrow() / FancyArrow is that it's shape is fixed in data coordinates, so appearance strongly depends on axis scaling and can look weird. IMHO this is broken by design.

This is solved by FancyArrowPatch. Unfortunately, we don't have a top-level function to only create a FancyArrowPatch. You can either create that patch manually and add it using add_patch() (which is a rather unusual API').

See https://matplotlib.org/stable/gallery/shapes_and_collections/arrow_guide.html#arrow-guide for a description of the existing possibilities.

annotate() OTOH does two things: 1) It creates a FancyArrowPatch 2) It can set a text at the end of the FancyArrowPatch. If you don't pass a text, annotate() is basically the missing top-level function to create the FancyArrowPatch. But I agree that "to create an arrow, make an annotation without text" is not great either.

Ideally, arrow() would return a FancyArrowPatch, but for historic reasons we're not there.

The above methods are for "create a single arrow from (x,y) to (x2, y2)". quiver() OTOH is used to visualize vector fields. 1) It's intended for many arrows. 2) It's mainly to show directions, not have given start and end points. You'd have to calculate a bit and set appropriate kwargs to get the start and end points at the positions you want.

@timhoffm
Copy link
Member

timhoffm commented Feb 3, 2022

It was decided in the dev call today that we want to deprecate arrow() because of its awkward dependence on the axis scales.
The deprecation period should be extended.

As an alternative, we want to introduce a similar replacement vector(x, y, dx, dy, ...) where the parameters are still in data space, but the arrow shape is not tied to the data. This should be implemented based on FancyArrowPatch which is also the basis for annotate() arrows. The additional parameters will be governed by the FancyArrowPatch capabilities, maybe be simply arrowprops like in annotate(). We may try to make an effort to choose their defaults in a way that will result in an arrow similar to arrow() with default settings. Whether that's possible and reasonable is to be tested.

The arrow() deprecation should point to vector() and annotate() depending on users needs.

@kmdalton
Copy link

kmdalton commented Feb 4, 2022

@timhoffm, plt.vector sounds like a great addition. Will it possible for the function to

  • add labels / handles to the current axis for plt.legend,
  • automatically choose colors from the prop_cycler, and
  • automatically set the x-y limits?

These are all default behaviors for plt.arrow but not plt.annotate.

@jklymak
Copy link
Member Author

jklymak commented Feb 4, 2022

We can easily extend the deprecation period.

I think discussion of the new vector method should perhaps get its own issue.

The arrow() deprecation should point to vector() and annotate() depending on users needs.

Just to be clear, I don't think the arrow deprecation needs to wait for vector to be created. annotate is a reasonable work-around, and arrow is fundamentally broken and a big foot cannon for users...

@jklymak jklymak force-pushed the mnt-deprecate-arrow branch from d6318a1 to 5801bfa Compare February 4, 2022 08:26
@timhoffm
Copy link
Member

timhoffm commented Feb 4, 2022

Just to be clear, I don't think the arrow deprecation needs to wait for vector to be created. annotate is a reasonable work-around, and arrow is fundamentally broken and a big foot cannon for users...

I beg to differ. #22390 (comment) explains that annotate is not a full replacement. We should have vector in place, so that the deprecation note can reference that as well and users have an alternative to turn to.

@jklymak
Copy link
Member Author

jklymak commented Feb 5, 2022

My point is that there is nothing to fully replace: arrow is fundamentally broken. The proposed vector is a great enhancement proposal, but whether anyone pushes that through or not doesn't obviate the need to get rid of arrow.

@timhoffm
Copy link
Member

timhoffm commented Feb 5, 2022

Of course there is on 1:1 replacement because the rendering is broken. But conceptually, you can do 70% with annotate() and will be able to do an additional 20% of the original functionality with vector(); numbers are only a very coarse estimate. Again see #22390 (comment) for why annotate() is not a full replacement.

When we deprecate, I want to be able to direkt users to the 70% + 20% alternative.

Arrow has been in for decades and has apparently quite some usages despite its shortcomings. There is no hurry to to take it away and we have to provide an as good as possible migration path for the existing users.

In practice, I suppose that we can get vector into 3.6 so there should be no difference to the user. But technically, IMHO the deprecation should depend on the vector introduction. If we don't get vector into 3.6, the deprecation should be delayed as well.

@jklymak
Copy link
Member Author

jklymak commented Feb 5, 2022

Sure, well we can close this, but I'm not holding my breath over the long-standing problem getting fixed.

@jklymak jklymak closed this Feb 5, 2022
@jklymak jklymak deleted the mnt-deprecate-arrow branch February 5, 2022 11:17
@QuLogic QuLogic removed this from the v3.6.0 milestone Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate Axes.arrow/pyplot.arrow
5 participants