Mhh, it's not easy, is it?! We need an arc whose radius is in screen coordinates (or better axes coordinates?), whose start and stop angle are dependent on other elements in the axes and whose position is in data coordinates. I wonder if it'd be better to defer much of the calculations to draw time?
I.e. creating a new artist with adraw method (or subclassingArc) to do what we need.

I can currently only promise to have a closer look in the next few days.

Mhh, it's not easy, is it?! We need an arc whose radius is in screen coordinates (or better axes coordinates?), whose start and stop angle are dependent on other elements in the axes and whose position is in data coordinates. I wonder if it'd be better to defer much of the calculations to draw time?
I.e. creating a new artist with adraw method (or subclassingArc) to do what we need.

I can currently only promise to have a closer look in the next few days.

No, it's not! I've found a way to keep the radius a constant in axes coordinates, but of course that messes up theta1 and theta2 for the arc. I'll keep working at it 😄

@ImportanceOfBeingErnest,

Okay, I think I got it! Regardless of the size of the figure or the axes, the arc is circular and at a constant pixel distance from the origin.

I agree with@ImportanceOfBeingErnest that doing the relevant calculations in draw() may work better (or better, fixTransform.inverted() so that it keeps track of the original transform, as discussed in some other issue...).
Still I guess the approach proposed here is reasonable in the meantime as an example (I wouldn't want it in the library itself though, there are problems e.g. if the user then changes some properties on the patch; then these changes get lost upon update).

When running the proposed code, I observe some very strange behaviour. Does this work as expected for you?

My thinking was that maybe one can subclassArc to let it do much of the work itself.

import numpy as npimport matplotlib.pyplot as pltfrom matplotlib.patches import Arcfrom matplotlib.transforms import IdentityTransformclass AngleMarker(Arc):    def __init__(self, xy, size, vec1, vec2, ax = None, **kwargs):        self._xydata = xy # in data coordinates        self.ax = ax or plt.gca()        self.vec1 = vec1 # tuple or array of coordinates, relative to xy        self.vec2 = vec2 # tuple or array of coordinates, relative to xy        super().__init__(self._xydata, size, size, angle=0.0,                 theta1=self.theta1, theta2=self.theta2, **kwargs)                self.set_transform(IdentityTransform())        self.ax.add_patch(self)    def get_center_pixels(self):        """ return center in pixel coordinates """        return self.ax.transData.transform(self._xydata)        def set_center(self, xy):        """ set center in data coordinates """        self._xydata = xy    _center = property(get_center_pixels, set_center)        def get_theta(self, vec):        vec_in_pixels = self.ax.transData.transform(vec) - self._center        return np.rad2deg(np.arctan2(vec_in_pixels[1], vec_in_pixels[0]))            def get_theta1(self):        return self.get_theta(self.vec1)            def get_theta2(self):        return self.get_theta(self.vec2)            def set_theta(self, angle):        pass             theta1 = property(get_theta1, set_theta)    theta2 = property(get_theta2, set_theta)            fig, ax = plt.subplots()ax.plot([2,.5,1],[0,.2,1])am = AngleMarker((.5,.2), 100, (2,0), (1,1), ax=ax)plt.show()

This is in pixel coordinates. So independent of the figure size and dpi it will have the same radius. I'm not sure in which units the radius should really be, to be honest. Maybe better in inches? or relative axes width? Points?

Oh wow, I didn't even try panning, but yes, mine does that as well 😆

I do like the idea of subclassing Arc; your solution is great! I don't think the radius should be a constant pixel or inch value, because wouldn't that result in the arc being drawn outside of the axes limits if the figure itself is made too skinny?

Hm, I copied your solution as is, and it doesn't seem to work on my system.

This is withsize set to 400. I checked the center coordinates withget_center_pixels and they are what they should be for the provided data, but it seems the center is outside of the axes. It also looks like something is off with the angles. I'll see if I can pinpoint the problem some time tomorrow.

I just tested and with matplotlib 3.0.0 and current master it works as expected for me, it doesn't work with 2.x though. Also I only tested with python 3.6. If you can share your versions one might more easily find out where the problem lies.

Yep, I was in the wrong virtual environment. My mistake!

So what is the next step? Your solution is certainly the better one, so I would be fine with you taking this PR. How do you feel about having the option to specify units for the radius?

Note that my interest is not to "take this PR" (not even sure what that means), but to help with finding a good solution here.

Indeed the main question is the one about the units of the radius. Pixels, as above, is not good, since it will not let the radius scale with dpi. Inches or points would be better. This would lead to a constant radius independent of the axes or figure size, which may, or may not be desired.
Alternatively units relative to axes or figure width (or height?) might be used. But this may also be undesired, e.g. the same content in a differently sized subplot will look different.
Allowing to set the units from the user side is definitely an option but makes all of this much more complicated. I suppose the way to go starting from my code above would be to make the width and height a property as well and use different transforms throughout the code depending on the chosen units. Also, one may think about adding a text label to the class, as often you want to annotate the angles and if working with different units, positioning that label might not be trivial for the user.

I might add that if all of this becomes too complicated, stepping back and writing a new Artist with its owndraw method from scratch for this case is not actually that hard, after all what we need is just part of a circle ([r*np.cos(phi), r*np.sin(phi)] within useful limits ofphi).

I think once we find an acceptable solution we may revisit the question of whether it makes sense to add this to the library itself - seen from how this is not as easy as anticipated, there may be true benefit of such function.

What I meant was that at this point, anything I add to my solution would be built off of yours, and I don't want to take credit where it's not due. Forgive me, but this is the first PR I've made to a library, let alone one as popular as Matplotlib, so I'm a little intimidated.

If it's okay with you, I'll try my best to expand on your solution to allow for the radius units to be specified by the user.

Yes, totally. A project like matplotlib lives on user contributions like yours. So please go ahead. I will label as "Work in progress" for now.

Okay, thank you for the guidance and for the opportunity!

I've modified your AngleMarker class to acceptunits as an argument, and the options arepixels orrelative for now. If set torelative, it behaves like my original solution did (without the strange behavior when panning) where the radius is directly proportional to the size of the axes.

However, to do this, I am setting thewidth andheight attributes of the class directly i.e.

self.width = ...

when these attributes actually belong to theEllipse ancestor. I'm setting them directly because there is no setter provided for those attributes.Ellipse has a setter for itscenter attribute, so why notwidth andheight? TheRectangle patch, on the other hand, does offerset_width andset_height methods. Do you know why theRectangle patch would offer these whenEllipse does not?

Do you know why the Rectangle patch would offer these when Ellipse does not?

There isn't any necessity for getters/setters in python and actually many people consider them as bad style. On the other hand, matplotlib has a tradition of using them in the public API. Of course the exact reason for the precise case of Rectangle and Ellipse is beyond my knowledge - possibly they have been written by two different people simply and noone has ever cared to add them. There is no technical necessity, right? It would only be for API consistency.

For the use case here it doesn't look like there is any problem resulting from missing getters/setters?

Right. Like you said, I had noticed them in other parts of the API, so it just caught my attention and I wondered if there was a known reason they weren't included forEllipse.

For my current solution, I'm using a callback for theresize_event of the figure, and I updatewidth andheight there - is there any other way to do this or is a callback the best option here? I set both width and height to the same fraction of the width of the axes in pixels, since the IdentityTransform ensures the arc will be circular as long as height = width. If the width of the figure is made skinny, the radius of the circle shrinks with it, and this looks great and exactly like it should. However, it doesn't shrink if the height is made skinny, which is actually great it the arc is mostly to the right or left of the center since you don't need to shrink the radius to keep the arc from going off-axes in this case. But if the arc is mostly above or below the center, this doesn't work. I tried fixing this by instead setting the height and width to the same fraction of thesmallest axes dimension in pixels, but this also leads to unwanted behavior e.g. if the arc is to the right of the origin and the height of the figure is skinny - the radius shrinks but it doesn't need to.

I'm pushing my solution as is for you to see, so forgive any weirdness/incompleteness. I look forward to your advice!

Well, if you use a callback anyways you can directly update everything through the callback. I guess you can do that, if you want, I just wouldn't actually mix the two concepts.
However my idea above was to not use a callback, but calculate the necessary parameters at draw time. For the width and height (which are identical here) this would be done similar to the angles, using the getter of the property.

I updated the code, see below. It'll now take pixels, points, axes width, height and the minimum or maximum of those as input units for the circle radius. All of those have some advantage and some drawbacks.

import numpy as npimport matplotlib.pyplot as pltfrom matplotlib.patches import Arcfrom matplotlib.transforms import IdentityTransform, TransformedBbox, Bboxclass AngleMarker(Arc):    """    Draws an arc between two vectors which appears circular in display space.    """    def __init__(self, xy, vec1, vec2, size=100, units="pixels", ax=None,                 **kwargs):        """        Params        ------        xy, vec1, vec2 : tuple or array of two floats            center position and two points. Angle marker is drawn between the            two vectors connecting vec1 and vec2 with xy, respectively. Units            are data coordinates.                size : float            diameter of the angle marker in units specified by ``units``.                units : string            One of the following strings to specify the units of ``size``:                * "pixels" : pixels                * "points" : points, use points instead of pixels to not have a                                        dependence of the dpi                * "axes width", "axes height" : relative units of axes                  width, height                * "axes min", "axes max" : minimum or maximum of relative axes                  width, height                          ax : `matplotlib.axes.Axes`            The axes to add the angle marker to                kwargs :             Further parameters are passed to `matplotlib.patches.Arc`. Use this            to specify, color, linewidth etc of the arc.                """        self._xydata = xy # in data coordinates        self.ax = ax or plt.gca()        self.vec1 = vec1 # tuple or array of absolute coordinates        self.vec2 = vec2 # tuple or array of absolute coordinates        self.size = size        self.units = units        super().__init__(self._xydata, size, size, angle=0.0,                 theta1=self.theta1, theta2=self.theta2, **kwargs)                self.set_transform(IdentityTransform())        self.ax.add_patch(self)            def get_size(self):        factor = 1.        if self.units=="points":            factor = self.ax.figure.dpi / 72.        elif self.units[:4]=="axes":            b = TransformedBbox(Bbox.from_bounds(0, 0, 1, 1),                                self.ax.transAxes)            dic = {"max" : max(b.width, b.height),                    "min" : min(b.width, b.height),                   "width" : b.width, "height" : b.height}            factor = dic[self.units[5:]]        return self.size * factor    def set_size(self, size):        self.size = size            def get_center_in_pixels(self):        """ return center in pixels """        return self.ax.transData.transform(self._xydata)        def set_center(self, xy):        """ set center in data coordinates """        self._xydata = xy        def get_theta(self, vec):        vec_in_pixels = self.ax.transData.transform(vec) - self._center        return np.rad2deg(np.arctan2(vec_in_pixels[1], vec_in_pixels[0]))            def get_theta1(self):        return self.get_theta(self.vec1)            def get_theta2(self):        return self.get_theta(self.vec2)            def set_theta(self, angle):        pass        _center = property(get_center_in_pixels, set_center)         theta1 = property(get_theta1, set_theta)    theta2 = property(get_theta2, set_theta)    width = property(get_size, set_size)    height = property(get_size, set_size)

Note that the code may not be PEP8 compatible as of now and may not be valid rst either. Also, it hasn't got any text attached - could be useful to add...

And then here is a code to look at the differences for the parameters. I think the problem you mention about the axes width/height depending on thedirection of the angle should be observable with this. There is no good solution, I fear. Hence providing all the options to the user is probably beneficial.
I haven't looked at that in detail myself though. (Attention, will create 56 files on disk when being run)

def testing(size=0.25, units="axes fraction", dpi=100, fs=(6.4,5), show=False):    fig, axes = plt.subplots(2,2, sharex="col", sharey="row", dpi=dpi,                              figsize=fs,gridspec_kw=dict(width_ratios=[1, 3],                                                         height_ratios=[3, 1]))    def plot_angle(ax, pos, vec1, vec2, acol="C0", **kwargs):        ax.plot([vec1[0],pos[0],vec2[0]],[vec1[1],pos[1],vec2[1]], color=acol)        am = AngleMarker(pos, vec1, vec2, ax=ax, **kwargs)        tx = "figsize={}, dpi={}, arcsize={} {}".format(fs, dpi, size, units)    axes[0,1].set_title(tx, loc="right", size=9)    kw = dict(size=size, units=units)    p = (.5, .2), (2, 0), (1, 1)    plot_angle(axes[0, 0], *p, **kw)    plot_angle(axes[0, 1], *p, **kw)    plot_angle(axes[1, 1], *p, **kw)    kw.update(acol="limegreen")    plot_angle(axes[0, 0], (1.2, 0), (1, -1), (1.3, -.8), **kw)    plot_angle(axes[1, 1], (0.2, 1), (0, 0), (.3, .2), **kw)    plot_angle(axes[0, 1], (0.2, 0), (0, -1), (.3, -.8), **kw)    kw.update(acol="crimson")    plot_angle(axes[1, 0], (1, .5), (1, 1), (2, .5), **kw)        fig.tight_layout()    fig.savefig(tx.replace("=","_") + ".png")    fig.savefig(tx.replace("=","_") + ".pdf")    if show:        plt.show()s = [(0.25, "axes min"), (0.25, "axes max"), (0.25, "axes width"), (0.25, "axes height"),     (100, "pixels"), (72, "points")]d = [72,144]f = [(6.4,5), (12.8,10)]import itertoolsfor (size, unit), dpi, fs in list(itertools.product(s,d,f)):    testing(size=size, units=unit, dpi=dpi, fs=fs)

I think at least for subplots on a grid, "axes max" is beneficial.

Axes min

Axes max

Yeah, I didn't like mixing a callback function with the draw-time calculations either. Thank you for showing me a better way, though I should have thought of that when I saw you doing the same thing with the angles originally. I'm definitely learning a few things from you!

I also like that you condensed the four lines

down to just

b = TransformedBbox(Bbox.from_bounds(0, 0, 1, 1), self.ax.transAxes)

I'll remember that in the future.

For the text, I think we'll want to make sure it doesn't overlap with anything else on the axes, but how do you feel about also having the font size change alongside the radius (maybe by a smaller factor)?

It'll be hard to make sure the text doesn't overlap with anything. Of course it should not overlap with the arc itself. I would vote against making the fontsize dependent on the arc size. This would be quite unusual. After all, the effort here is to make the angle marker size constant in some units, such that you get similarly sized markers. Of course the user should be able to set all the text properties to his liking. I see two options:

1. Allow for amatplotlib.text.Text as input and let the class manage its position
2. Allow for a string as input and in addition a keywordtextproperties that can be used to set the text properties. An annotation would then be created internally.

There are also still some open question as to how to place the text:

or introduce a keyword argument to steer that behaviour.

Would you recommend against having AngleMarker inherit from Text in addition to Arc?

No, that might be possible (you need to check if there are attributes that clash). Also consider usingAnnotation instead ofText, which might come handy with all the different coordinate systems.

Okay, that's the route I had started to go, so I'll continue. I haven't found any attributes that clash.

Okay, so AngleMarker now inherits from both Arc and Text. I'm still working out the general placement of the text, but in my most recently pushed commit you can see that its position relative toself._center never changes as you pan and zoom.

I'm going to try to have it always appear just outside of the arc halfway between theta1 and theta2. Or do you think it would make sense to allow the user to enter an angle for the position of the text?

I think the angle might always be half of the angle between the vectors. So if anything should be custumizable it would be the distance from the center - either the two options shown as picture above or more generally any distance in the units ofunits or in units ofsize.

I've moved this to draft until@timhoffm's review is addressed.@daniel-s-ingram did you want to see this through?

@daniel-s-ingram If we don't hear from you within a week, we'll take over and do the requested changes ourselves. This is almost there and it would be a pity if it gets stalled and lost.

Since it's been about 3 weeks, I've gone ahead and rebased this, squashing the flake8 commits, and fixed the suggestions, plus a few prose changes from myself.

Let's squash into one commit. This is a newly added standalone example and does not need it's creation history reflected in git.

Sure, I'd say just do that on merge.

I'd favor calling thisAngleLabel orAngleIndicator, but won't insist because it's only an example and not part of the Matplotlib API.

I renamed toAngleAnnotation, since it's both a little angle arc and a text label, though I guessAngleIndicator might work too.

Twhanks to all contributors!