Skip to content
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

Vector #22435

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Vector #22435

wants to merge 9 commits into from

Conversation

kmdalton
Copy link

@kmdalton kmdalton commented Feb 9, 2022

PR Summary

This Draft PR is a first pass at implementing a vector method (see #22390) for the Axes class. Ideally someone can build on this to make a viable plt.arrow successor.

It uses FancyArrowPatch to draw the arrow. Right now the ArrowStyle is limited to "simple". It would be trivial to support "fancy". Other ArrowStyles will be more challenging as they diverge in parameter names.

This version supports legend labels out of the box through the Axes patch handler, and it automatically adjusts the axis limits. The arrow sizing can be adjusted in points. The scaling logic is based on the FancyArrowPatch code in plt.Annotate here.

Much of the credit goes to @ianhi for working out the details.

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

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

Comment on lines +6807 to +6812
ms = vect._mutation_scale
stylekw = {
"head_length": kwargs.get("head_length", 12) / ms,
"head_width": kwargs.get("head_width", 12) / ms,
"tail_width": kwargs.get("tail_width", 4) / ms,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For context this numbers are based on how annotate styles it's arrows.

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
@story645 story645 added status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. topic: arrow New feature labels Feb 9, 2022
@@ -6777,6 +6777,45 @@ def hist(self, x, bins=None, range=None, density=False, weights=None,
else "list[Polygon]")
return tops, bins, cbook.silent_list(patch_type, patches)

def vector(self, dx, dy, x=0., y=0., **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def vector(self, dx, dy, x=0., y=0., **kwargs):
def vector(self, x, y, dx, dy, **kwargs):
  • We want to keep the API similar to arrow()
  • While from a mathemetical point of view, you can have a direction vector only specifying dx, dy; but we need to plot it somewhere and there is no reasonable default. I don't think there is a real use case where you are not interested how the arrow is positioned relative to other plot elements.
  • If you have all four parameters x, y, dx, dy is the natural order.

Copy link
Author

Choose a reason for hiding this comment

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

This may be an peculiarity of my field, but crystallographers frequently plot arrows in the complex plane that are by default rooted at zero. As evidence that I am not alone, have a look at an image search for Argand diagram. From my perspective, x=y=0 is a sensible default. I can understand how this might not be universal.

I find vector(tail_x, tail_y, head_x, head_y, ...) is also a natural four-parameter representation. Is that one the table?

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

This seems a good start, but does not, so far as I can tell, allow for arrays of x, y, dx, and dy to be passed, whereas that is a natural thing to do. In which case you probably want a PolyCollection?

You also have not taken into account our units system - x, dx etc need to be able to take datetime and other units, and needs tests for these. Maybe see errorbar (?) for an example of deltas being used with the unit system.

@kmdalton
Copy link
Author

@jklymak , I don't think plt.vector needs to support arrays for the following two reasons

  • plt.arrow does not
  • plt.quiver exists for this purpose

@jklymak
Copy link
Member

jklymak commented Feb 10, 2022

Indeed plt.quiver provides this functionality, so why do we need vector? It's easy enough to pass a single x, y, u, v to quiver.

@ianhi
Copy link
Contributor

ianhi commented Feb 10, 2022

Indeed plt.quiver provides this functionality, so why do we need vector? It's easy enough to pass a single x, y, u, v to quiver.

I tried this out a bit and quiver is unfortunately not good substitute. Trying to replicate vector requires two extra kwargs and still doesn't replicate it. It also doesn't follow the color cycle

import matplotlib.pyplot as plt
import numpy as np

fig, ax = plt.subplots()

x = .5
y = .5
dx = 1
dy=10
ax.vector(dx, dy, x ,y, label='vector')
shift = .05
ax.quiver(x+shift,y+shift, dx, dy, angles='xy', scale=np.linalg.norm([dx,dy]), label='quiver')
plt.legend(loc=2)
plt.show()

gives
image

@jklymak
Copy link
Member

jklymak commented Feb 10, 2022

Ok then, there is probably a need for vector to accept more than one set of data

@timhoffm
Copy link
Member

We've discussed this method in the dev call: https://hackmd.io/jd_7FjxNQ4y7XgNknvmvGQ#vector-API

Basic ideas:

  • vector() is a replacement for arrow() and thus try to follow its API where possible.
  • vector() is a method wrapper for creating a FancyArrowPatch.

Summary:

  • we want vector(x, y, dx, dy, **kwargs); all kwargs are passed through to FancyArrowPatch. The non-kwarg parameters match arrow().
  • vector should only accept and draw one arrow. Accepting more causes some issues either way. You get either return type instability or always have to return a list of FancyArrowPatches, which is awkward for the common case that you only draw one. Note also that PatchCollection does not work with FancyArrowPatch (and this is not easily fixable).
  • There's been a hot discussion whether vector should follow the color cycle or not (and if so the line or the patch color cycle) (and if not should there be an additional `rcParams['vector.color'] to make it configurable.

Long story short, the color topic needs more investigation, we haven't reached a decision yet.

@kmdalton
Copy link
Author

As a user, I would be very frustrated if plt.vector did not follow the color cycle. I feel like this is a sensible default and can always be overridden through the "color", "ec", and/or "fc" kwargs. I guess I am missing the counterargument.

@ianhi
Copy link
Contributor

ianhi commented Feb 11, 2022

There's been a hot discussion whether vector should follow the color cycle

I second @kmdalton that it should follow a color cycle. I find methods that don't to be extremely frustrating.

or not (and if so the line or the patch color cycle)
Long story short, the color topic needs more investigation, we haven't reached a decision yet.

Probably not best discussed here but I've always wished that there was one global color cycle. It's disconcerting that scatter, plot and hist each have their own color cycle. That aside, I would advocate for the plot color cycle.

@jklymak
Copy link
Member

jklymak commented Feb 11, 2022

If we are only making one arrow at a time, then I'm not convinced adding a new method is worth it just for the autolim and colorcycle behaviour. The colorcycle is dubious; I don't understand why we would want single arrows cycling their colors by default (groups of many arrows, sure). Similarly if I'm just adding one arrow at a time I typically have other data that I have plotted and that data set the limits for me.

@kmdalton
Copy link
Author

If we are only making one arrow at a time, then I'm not convinced adding a new method is worth it just for the autolim and colorcycle behaviour. The colorcycle is dubious; I don't understand why we would want single arrows cycling their colors by default (groups of many arrows, sure). Similarly if I'm just adding one arrow at a time I typically have other data that I have plotted and that data set the limits for me.

Here's a plot I made last week using plt.arrow to demonstrate complex addition for my students.

image

I had to manually specify colors, limits, and otherwise override the broken default kwargs in plt.arrow. The whole process was frustrating and time-consuming. As you've noted, @jklymak , plt.arrow doesn't use FancyArrowPatch so the result isn't very aesthetically appealing either.
With the version of vector in this PR, you can already do this sort of plot in just a few lines with mostly default params.
image

import matplotlib.pyplot as plt
import numpy as np

F1, F2 = np.random.random(2) + 1j*np.random.random(2)

fig, ax = plt.subplots()

dx,dy,x,y= np.real(F1),np.imag(F1),0., 0.
ax.vector(dx, dy, x, y, label='F1')

dx,dy,x,y= np.real(F2),np.imag(F2),np.real(F1),np.imag(F1)
ax.vector(dx, dy, x, y, label='F2')

dx,dy,x,y= np.real(F1+F2),np.imag(F1+F2),0., 0.
ax.vector(dx, dy, x, y, label='F1+F2')

plt.xlabel(r"$\mathbb{Re}$", size=16)
plt.ylabel(r"$\mathbb{Im}$", size=16)
plt.grid(ls='-.')
plt.legend()
plt.show()

@jklymak
Copy link
Member

jklymak commented Feb 11, 2022

The relevant comparison is not plt.arrow which we all agree is broken, but plt.annotate.

I find your diagramming use case very specialized that you could easily write a little annotate wrapper for. I don't feel it justifies a whole new api element. But I'm just one vote. If others on the dev team think this is a good way to go, I'm not going to block it (unless it lacks unit support)

@timhoffm
Copy link
Member

I'm still not clear what the right way forward is. Let's maybe take one step back from what vector should do

Basic ideas:

  • vector() is a replacement for arrow() and thus try to follow its API where possible.
  • vector() is a method wrapper for creating a FancyArrowPatch.

to what is needed:

  1. Current users of arrow() need an alternative, that is reasonably simple to switch to.
  2. It should be possible to create a single FancyArrowPatch arrow using a function.
  3. There is a desire to be able to draw a set of arrows at once.

Discussion

RE 3: is out of scope here. Likely this wants a collection-type Artist, which we currently don't have for arrows (it was mentioned in the dev call that PatchCollection does not work with FancyArrowPatch, and that's not easily fixable). So let's save that for later.

RE 2: "To create an arrow, create an annotation without text." is too convoluted. Semantically, a data arrow and an annotation are different things (and they may have different behaviors, e.g. concerning color-cycling). We need a dedicated function. Whether that'd be vector or fancy_arrow.

RE 1: I'm pulling back a bit on the need to follow the arrow() API, as long as everything is still possible and it's reasonably simple to switch to the new API.

@timhoffm
Copy link
Member

We've discussed this extensively again in the dev call today: https://hackmd.io/jd_7FjxNQ4y7XgNknvmvGQ#Arrow

Conculsions:

  • Definitly not doing multiple here. This can be done with quiver in a reasonable way but definitly needs to be better documented.
  • Change signature to def vector(self, start, *, delta=None, end=None)
    • require exactly 1 of delta or * end*
  • should also add the start, end, delta setter/getters to FancyArrowPatch
  • color cycle: yes and line cycle
  • legend: yes
    we at least check and document what these do or do not get picked up by plt.legend(['a', 'b', 'c'])

@jklymak
Copy link
Member

jklymak commented Feb 19, 2022

Sorry to miss the discussion - however, what was the argument for vector, other than to replace arrow? For multiple arrows we have quiver; for single arrows we have annotate. Our API problems are usually caused by too many similar ways to achieve the same thing (see pcolor,pcolorfast, pcolormesh), so I still don't understand why we want to add something that is basically just annotate with no option for accompanying text.

@timhoffm
Copy link
Member

timhoffm commented Feb 19, 2022

for single arrows we have annotate.

As discussed above "To create an arrow, create an annotation without text." is awkward and not intuitive/discoverable. Moreover, they are conceptually different. Annotations are decorations, while we came to the conclusion that vector arrows should be considered as data. This implies that vector arrows take part in color cycling and can be added to the legend.

@jklymak
Copy link
Member

jklymak commented Feb 19, 2022

This will be the only place in the library where "data" is only allowed to be represented by one set of values per method call.

@timhoffm
Copy link
Member

We don't have a suitable artist for multiple FancyArrowPatch (it doesn't work with PatchCollection because it needs special draw-time logic: an positioned in data space but it's shape must be drawn in screen space. 🤷 There's always the option to extend vector() later, but let's start small for now.

@kmdalton
Copy link
Author

Is there consensus about the spec for this method from the devs or not? I'm sure myself or someone else will continue to spend time on this feature, and I would hate to see the end result of that effort be a PR won't be merged. Can someone clarify this for me?

@jklymak
Copy link
Member

jklymak commented Jan 24, 2023

@kmdalton sorry for the contention on this one. Just to put the process in context here, a dev can block a PR if they really think it's wrong or misguided, and even that can be cleared by other devs, usually after a discussion.

Second a PR needs two people who would be willing to merge it should it come up to standard (API OK, documentation good, and has tests). I think in this case if you have two people who are willing to see this through review then this will get in. I'm not blocking, but feel I should note that I think this is a mistake in its current form.

@story645
Copy link
Member

Second a PR needs two people who would be willing to merge it should it come up to standard

I'll approve/merge an implementation with the signature in #22435 (comment) and test/docs. I use arrows all over the place and the shrinking issue in particular is how I ended up cycling through all the arrow methods. And like I need to annotate these arrows rather than use them to do annotation...

@jklymak
Copy link
Member

jklymak commented Jan 24, 2023

I think some of the confusion here comes from folks being tripped up by arrow. We should deprecate and remove arrow - it's fundamentally broken. But I don't see how adding another method to cycle through is going to help the discoverability problem of "I want to draw an arrow; there is an arrow method, darn its broken, now what do I do?" The docs for arrow already says to use annotate, if that didn't help, how will saying to use vector help any better?

Again I will be less obstreperous if a) this method just wraps annotate or b) annotate is re-factored to use this method to generate the arrow. I just don't want two diverging APIs, vector and arrow that do exactly the same thing but develop completely diverging syntaxes for doing it.

@story645
Copy link
Member

story645 commented Jan 24, 2023

just don't want two diverging APIs, vector and arrow that do exactly the same thing but develop completely diverging syntaxes for doing it

As far as I know, deprecating arrow is just waiting on a vector replacement to go in. Different names mostly b/c of the different signatures. ETA: #22390

a) this method just wraps annotate or b) annotate is re-factored to use this method to generate the arrow.

They both wrap FancyArrowPatch, so the base patch object stays the same. This function just removes all the text placement logic that is the heart of annotate and also in my opinion what makes annotate kinda intimidating/overwhelming for the sort of simple "draw an arrow" tasks arrow was used for. (as an aside, I still want FancyArrowPatch to grow a .set_text method #22223 (comment)) Also I think differences in defaults, like shrink, are kinda core to using arrows for annotations vs to plot data/diagrams.

The docs for arrow already says to use annotate, if that didn't help, how will saying to use vector help any better

At least to me, annotate is a labeling function rather than a data->visual encoding function and that conceptual difference of what type of visualization task they're designed for makes me feel really awkward using annotate for non-labeling tasks, especially since it's annotate with knobs off. I think keeping annotate: maybe labeling + maybe arrow + maybe box separate from vector: just arrows allows us to better define scope, keep folks who only need vectors away from annotate (way too many knobs there), and gives us more flexibility to shape each API for its use case.

@jklymak
Copy link
Member

jklymak commented Jan 25, 2023

A common complaint about Matplotlib is that there are too many ways to do the same thing, and the proposal here is to add another way to do something already accessible via annotate. I think such an addition should have a compelling case, and hew to the existing API as closely as possible for the sake of consistency.

@story645
Copy link
Member

story645 commented Jan 25, 2023

A common complaint about Matplotlib is that there are too many ways to do the same thing, and the proposal here is to add another way to do something already accessible via annotate

Um so I don't think they're two ways to do the same thing, I think they're two different things (annotating and encoding1) that may generate the same visual element (an arrow). Kinda like how plot and vlines can both create vertical lines, or vlines+scatter = stem, or matshow is shifted imshow. And I think a lot of the confusion for users is also not so much in the multiple ways to do things, it's that we could be much clearer about which way matches their use case and therefore as part of the implementation has the defaults (eta: baked in semantic assumptions) they expect for their visualization task.

For example, this new vector method would get an entry in plot types and maybe we should add a new section there and move all the annotation type plotting methods - spans, fills, annotate-there.

Footnotes

  1. A Multi-Level Typology of Abstract Visualization Tasks

@timhoffm
Copy link
Member

proposal here is to add another way to do something already accessible via annotate.

@story645 has clearly laid out the arguments why vector is different from annotate and why we need it in addition. Please answer specific to these arguments if you think the are invalid.

hew to the existing API as closely as possible for the sake of consistency.

This is generally a good idea. We‘ve started from there in the API discussion rounds. However we realized that a) the current arrow API has deficits and b) changed behavior is desirable (e.g. color cycling). Because of b) vector is not a 100% compatible drop in replacement anyway. Then, taking a slightly larger step and improve the API can be reasonable. Overall, we can choose to prioritize either compatibility or improving the API and functionality. In our discussions there has been a consensus to go with le latter in this case.

@ianhi
Copy link
Contributor

ianhi commented Jan 25, 2023

a dev can block a PR if they really think it's wrong or misguided, and even that can be cleared by other devs, usually after a discussion.

To my understanding this discussion happened at the weekly meeting and a consensus was reached as presented in #22435 (comment). Is this not the case?

As a broader point: It's difficult to work on PRs like this if there isn't clarity about what constitutes a consensus. As an experienced contributor I find that I can stick with it, but were I newer then the uncertainty as to whether or not there has been a conclusion of if this feature will make it in in any form would be very difficult to deal with.


I've just pushed an update that I think matches the spec as laid out in #22435 (comment) with the small exception that I only added a delta setter/getter as set_position already gives the ability to change the start and end (although with naming of posA, and posB).

Happy to add tests once there is clarity that this is direction this PR should take.

@ianhi
Copy link
Contributor

ianhi commented Jan 25, 2023

With current implementation this example code:

import matplotlib.pyplot as plt
import numpy as np

F1, F2 = np.random.random(2) + 1j*np.random.random(2)

fig, ax = plt.subplots()

dx,dy,x,y= np.real(F1),np.imag(F1),0., 0.
ax.vector((x,y), delta=(dx, dy), label='F1')

dx,dy,x,y= np.real(F2),np.imag(F2),np.real(F1),np.imag(F1)
ax.vector((x,y), delta = (dx, dy), label='F2')

dx,dy,x,y= np.real(F1+F2),np.imag(F1+F2),0., 0.
ax.vector((x,y), delta=(dx, dy), label='F3')

plt.xlabel(r"$\mathbb{Re}$", size=16)
plt.ylabel(r"$\mathbb{Im}$", size=16)
plt.grid(ls='-.')
plt.legend()
plt.show()

gives:

image

@timhoffm
Copy link
Member

timhoffm commented Jan 26, 2023

To my understanding this discussion happened at the weekly meeting and a consensus was reached as presented in #22435 (comment). Is this not the case?

Historic note: We had at least two weekly meetings on the topic and positions have changed throughout and between the meetings (the topic is compicated and we have to balance different aspects). #22435 (comment) was the consensus of the last meeting. I don't remember exactly, but I think @jklymak has not been part of that meeting.

As a broader point: It's difficult to work on PRs like this if there isn't clarity about what constitutes a consensus. As an experienced contributor I find that I can stick with it, but were I newer then the uncertainty as to whether or not there has been a conclusion of if this feature will make it in in any form would be very difficult to deal with.

I definitively agree. This topic turned out to be more complex than we initially thought. We had a first decision on the API, which then was labelled "good first issue". Only after the PR came in and all details were mapped out, we partially realized that the initial decision wasn't quite right, which resulted in the following controversial discussion.
The general appoach of labelling "good first issue" was right because at that time, we thought it's all decided and the PR is only doing. We may be more careful with that label on anything that needs non-trivial API decisions in the future, even if we think they are set.

@jklymak
Copy link
Member

jklymak commented Jan 26, 2023

Please answer specific to these arguments if you think the are invalid.

Those arguments are based on "encoding" of the method for use case, which is completely subjective, and boil down to "I don't think of adding arrows that join data as annotating". I think we should guard against the API being expanded just to fit different mental models of how to add information to a plot. Expanding the API should add new non-niche actual functionality that is not otherwise available, or it should be significantly more convenient for a significant proportion of our users.

What's more, I don't agree that adding arrows, one at a time, is not annotating the plot, regardless of the intention. By this logic we should also allow plot_data_with_text(x, y, string) that plots a string at x, y to represent the data at that point. Is it a valid thing to do? Sure! Is it functionally any different than text(x, y, string) or annotate(string, (x,y))? No. You could add autolim behaviour, have the text color cycle through the color cycle, and make it show up in the legend. But it's still exactly the same thing, and I doubt we would allow it as new API.

What I am arguing against is having the same object put on the plot by different APIs. If you need the autolim behaviour (which is the most useful in my opinion), that could be added to annotate. I'm skeptical about the color cycling, but again, could be added to annotate, as could allowing the arrow patch be part of the legend. That leaves us with the slight awkwardness that annotate has a default argument of text, and the xy, and xytext not being great names for specifying where an arrow goes.

However, plotting one vector at a time and cycling through a colorcycle and adding the vectors to a plot is extremely niche. I have never seen a plot like that outside of a physics or math text book, and even then the vectors are typically the same color, and labeled in-situ rather in a legend. I don't mind, at all, Matplotlib being used for diagraming vectors this way, but I don't think it merits a new API when it can relatively easily be achieved via annotate.

If you cannot remember that annotate is a great way to draw individual arrows, adding a helper method to your helper library is surely a nice way to work around awkwardness in the API if you don't like the defaults and you do something often enough.

@story645
Copy link
Member

story645 commented Jan 26, 2023

If you need the autolim behaviour (which is the most useful in my opinion), that could be added to annotate. I'm skeptical about the color cycling, but again, could be added to annotate, as could allowing the arrow patch be part of the legend. That leaves us with the slight awkwardness that annotate has a default argument of text, and the xy, and xytext not being great names for specifying where an arrow goes.

This is essentially a suggestion to grow the annotate implementation so that it supports the behavior expected of a visualization function instead of the proposal here of factoring this behavior into a well scoped vector function. Annotate is plenty complex because of the coordinate labeling logic and signature related to it, and the arrows mostly seem to be a by product of how matlab does it.

ETA: The point @timhoffm and I are trying to make, which you seem to support here, is that visualizing data as arrows has different requirements than drawing arrows for labeling; therefore one way to make this easier on the user (ETA: and maintainer) is to encode these different needs and expectations in different functions.

@timhoffm
Copy link
Member

I think this need‘s a live discussion to resolve. Unfortunately, I currently cannot commit to a dev call. You may discuss this without me. I support @story645‘s position. (Or we’ll have to defer this a bit- 3.8 is still somewhat away.)

@jklymak
Copy link
Member

jklymak commented Jan 26, 2023

Again, I don't think this meets my criteria of what should be added to the API. If no one else is concerned about the cruft, you should feel free to move forward.

@anntzer
Copy link
Contributor

anntzer commented Jan 26, 2023

I apologize for not making it to the calls these days (a bit overwhelmed with work), but I generally with @jklymak's points:

If you need the autolim behaviour (which is the most useful in my opinion), that could be added to annotate. I'm skeptical about the color cycling, but again, could be added to annotate, as could allowing the arrow patch be part of the legend. [...] However, plotting one vector at a time and cycling through a colorcycle and adding the vectors to a plot is extremely niche.

I just performed the following poll to n=1 matplotlib user: "In matplotlib there are functions which auto-advance the color (e.g. plot()) and functions which don't (e.g. text()). There is currently a proposal to add a function for drawing arrows (which is currently possible but a bit complicated); do you expect that this function should auto-advance the color or not?" (This question is specifically formulated so that the pollee doesn't need to know advanced concepts like "color cycle" other than as "what plot() does", and also assumes that the pollee doesn't know about the already existing arrow() method -- I believe both assumptions are true for the vast majority of matplotlib users. I also don't think the formulation is particularly biased.)

I got the response "no the arrows should obviously all be black, why would you want them of various colors?" (followed by a rather salient "Usually if you have two line plots which are the same color it's impossible to tell which is which, whereas usually arrows are self-evident given where they point to and thus don't need different colors". Naturally I realize that there are exceptions, but that was the intuition of the user I polled.). Naturally that's n=1 but I could easily poll more people and would suggest that others do the same. Perhaps my intuition is completely off, but I would bet that the vast majority of users would respond "the arrows should all be black".

Note that my point is not really "we should not add vector()"; it's more "it should be more like annotate".


Edit: Current polling status: 3 users against color-cycling, 0 in favor of it.


As a side point, if we are going to make the "it can be added to legends" feature a selling point for vector(), then it should get a proper legend handler so that the legend entry is also an arrow; having the legend entry be a rectangle of solid color looks quite sloppy to me.

@story645
Copy link
Member

story645 commented Jan 27, 2023

We didn't discuss it on the call so nothing missed. I'm currently too burnt out from stuff happening at home to intentionally add a contentious discussion.

if we are going to make the "it can be added to legends" feature a selling point for vector() then it should get a proper legend handler so that the legend entry is also an arrow

💯 and also I think this is another reason for factoring all this stuff into vector, b/c labeling-annotations generally serve the same "identify/add context to" role as legends and therefore probably don't need this logic either. (ETA: With the assumption that vector returns a FancyArrowPatch while annotate returns an text.Annotatation object, so the handler would I think be for FancyArrowPatch)

I think it should participate in the color cycle for the sake of consistency, but also a "help us design new API" banner at the top of the docs/as poll in the sidebar could be a fun little user engagement project.

@anntzer
Copy link
Contributor

anntzer commented Jan 27, 2023

it should participate in the color cycle for the sake of consistency

Text doesn't participate in the color cycle, so consistency isn't everything. Yes, I realize that arrows can be "data" rather than "annotations", but (in agreement @jklymak's point above, I believe) I believe that if you poll a few users and ask them a method that draws arrows should participate in the color cycle or not (which is essentially asking "is it more like text() or more like plot()", without getting into abstract concepts like data vs. annotations) they would say "no" (i.e. they see it as annotations).

@timhoffm
Copy link
Member

Re: color cycle participation (semi-OT):

Aside from fundamental considerations, part of the argument for color-cycling was "it's easy to opt out by giving an explicit color; you cannot opt-in". Possibly we should make it easier to opt in; e.g. supporting color='next' or color='from_cycle' or similar. This needs careful consideration though, because cyclers are more complex than only color.

@story645
Copy link
Member

story645 commented Jan 27, 2023

Looked it up and a major reason for opting into color_cycle is that's what arrow did #22390 (comment) and this is supposed to be the replacement 😓

but also @kmdalton and @ianhi what do you all think since you use these in your work?

ETA: on the legend patch - I think a FancyArrowPatchHandler can be created separate from this PR/as a follow up, and that the regular patch handling is good enough.

@kmdalton
Copy link
Author

I'm a bit behind on this thread, but to answer @story645, I would prefer that vector

  • Follow the color cycle
  • Work with plt.legend

My use case often looks like what @ianhi shared in his last post.

@jklymak
Copy link
Member

jklymak commented Jan 27, 2023

It would be nice to see prior work - a published example of this type of plot and/or another library that does this.

@kmdalton
Copy link
Author

There are countless examples in Bernhard Rupp's textbook, but I will not post them here because I do not have the copyright. Here are some fairly typical examples online. Most often authors use color coded arrows which are annotated with text. When I make these sorts of plots for lectures, I prefer to use legends to cut down on clutter. Frequently we use circles to represent a set of complex numbers with a particular amplitude and offset.

@jklymak
Copy link
Member

jklymak commented Jan 27, 2023

An example from Rupp is at https://www.ruppweb.org/Xray/tutorial/vectordiag.htm. Here is a journal article https://journals.iucr.org/d/issues/2003/11/00/ba5050/ba5050.pdf that makes liberal use of these. Note that these are called vector diagrams or Argand diagrams and they all, almost uniformly, have a bunch of labelling, arc circles etc. They also tend to key two or three colors with semantic meaning. They also never seem to have quantitative information on the axes.

@melissawm
Copy link
Contributor

Hello, friends! What are the action items here? Should we add this for discussion at the community meeting?

@ianhi
Copy link
Contributor

ianhi commented Mar 31, 2023

I think the last thing I considered blocking was #22435 (comment) and #22435 (comment) which imply that a live discussion is needed to finalize the api. After that there will need to be (1) tests, (2) a custom legend patch as a follow up PR

FWIW I think Kevin and I got a bit disheartened/overwhelmed by the level of back and forth. So in the mean time we have made and are using mpl-arrow. Though I still believe that long term this has a place in the core library (e.g. we had to use some private methods to get it work correctly).

n.b. I appreciate that it's difficult to get API "right" and thus understand the necessity of the back and forth, but nonetheless found it disheartening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New feature status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. topic: arrow
Projects
Status: Needs decision
Development

Successfully merging this pull request may close these issues.

None yet

10 participants