GeezerVison

Hi everyone,

I was just wondering whether someone would be willing to have a look at this PR? I feel like it's in a complete or nearly-complete state but need some guidance - the two failing CI jobs seem to be:

• PR cleanliness:
  The added unit test was written first by @Abitamim, and then expanded by myself. When I expanded the test to check the newly-added reversed arrow heads, I changed both the test name (temp_test_boxarrow → test_boxarrow_adjustment) and the name of the output image (roadsign_test_image.png → boxarrow_adjustment_test_image.png) to reflect this. The 'file both added and deleted' error given seems to be a result of adding then deleting roadsign_test_image.png.
• Python 3.11 on macos-13:
  The failing test here (test_blitting_events) is marked as flaky, with the comment # subprocesses can struggle to get the display, so rerun a few times. I feel it may be that this was just a random instance of the test failing multiple times in a row; unfortunately I don't have access to macos-supported hardware for rerunning this locally, so I was wondering if someone would be able to take a look.

Any help would be greatly appreciated. Thanks!

PR-cleanliness: This is a check so that we don't merge commits with unneeded images, which would increase repo size. You can either squash and force-push. Or ignore this and we'll squash-merge at the end.

Ignore the macos test failure. It's unrelated.

Thank you for getting back so quickly! Would it be okay if you handled the squashing - I'm relatively new to git, and I'm not sure of the best method to use for squashing to keep a relatively clean but traceable commit history?

Since I'm going to need to make some relatively major changes to the contents of this PR (a redo of the arrow drawing code and a rewrite of its unit test), would it be better for me to amend my last commit (d9b0b32) as suggested in the contributing guide, or make a few new commits? I'm not quite sure how to add more commits to an open PR - do I just push the changes to my development branch?

@CharlieThornton33 yes you can just push more commits to your current branch and they will appear in this PR. Since you already have several commits they will ultimately need squashing one way or the other (and I see you already asked that we handle that), so I do not think you gain anything from the —amend option.

I've just redone the padding (only for LArrow and RArrow for the moment), and I think the issue with the text escaping the outline has been fixed. I set the arrow body to dynamically increase in length to make sure there's always a gap of at least pad between the edge of the text closest to the arrow head and the position where that edge would first intercept the outline if moved towards the tip:

import numpy as np
import matplotlib.pyplot as plt

fig, ax = plt.subplots(figsize=(3, 8))

t0 = ax.text(0.5, 0.9, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=-90",
                      fc="lightblue", ec="steelblue", lw=2))

t1 = ax.text(0.5, 0.7, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=70",
                      fc="lightblue", ec="steelblue", lw=2))

t2 = ax.text(0.5, 0.5, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=100",
                      fc="lightblue", ec="steelblue", lw=2))

t3 = ax.text(0.5, 0.3, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=130",
                      fc="lightblue", ec="steelblue", lw=2))

t4 = ax.text(0.5, 0.1, "Text",
            ha="center", va="center", rotation=0, size=30,
            bbox=dict(boxstyle="rarrow,pad=0.3,head_width=1.5,head_angle=170",
                      fc="lightblue", ec="steelblue", lw=2))

plt.show()

It's also a bit concerning that the CI workflow seems to run on every commit to this PR - am I okay to disable tests in my commit messages until the one where I write the new test to save on CI resources?

I've just noticed - all of the pytest test runs are about to fail because I forgot to remove the flawed test_boxarrow_adjustment test. Is there any way to cancel a CI test run?

@CharlieThornton33 I wouldn't worry about the CI failing, but if they are still running when you push another commit they will be interrupted and re-started.

I'm a bit confused about how to test one of @QuLogic's review notes:
https://github.com/matplotlib/matplotlib/pull/29998/files/2b83affd050cf92231756104a6abc7266d2e0565#r2191691643

How would it be possible to test modification of a public attribute? Would it just be initialising the box with fig.text, then modifying the attributes (I don't know if setter/getter methods exist for these; I'll check now) before the test ends and presumably calls __call__ to render the bbox?

I believe @QuLogic's comment is fuelled by the idea: What if somebody created a boxstyle class and then modified it

arrowstyle = LArrow(head_angle=45)
plt.text(0, 0, "text", bbox={'boxstyle': arrowstyle}
arrowstyle.head_angle=30
plt.show()

Since the class stores all parameters in attributes as is and all of the logic is in __call__, that will work, even though it should be a very rare use case. I would not bother to add a test for this. One caveat: The constructor does the range checking, so that you could set the attributes to invalid values. Overall, I'd tolerate that. The alternatives would be to do the validation in __call__ instead of __init__, which is a less good user experince as the error happens further away from the offending code. Or you would have to guard the attributes via a property setter, but I think it's not worth the added complexity. The worst that will happen is that for the very very rare case that users modify an existing BoxStyle and use an invalid value, that will blow up later with a more cryptic error message.

Hi,

I was wondering how I should proceed with this PR - I can't think of anything else that needs finishing for it.

The result looks good. Though, I would like to understand the calculation #29998 (comment). Relying on empirical values that we don’t understand poses a danger that they are incorrect for some cases.

We need to debug the logic step by step and either extract the coordinates to draw the picture by hand, or draw points/rectangles (see e.g. #29833 (comment)) to understand what’s going on.

I guess it depends on how close we want it to match the old version.

my main concern is consistency. The new code must produce reasonable results, I.e. text must be inside the arrow outline, for all parameter combinations. The code should be not too complex (try to avoid too much special-casing). Matching existing behavior is only third - note that these are outlines around a text, and are defined via the text position. The tip position and arrow size are not explicitly set but depend on font metrics and size. I therefore think it is permissible to not exactly reproduce existing behavior.

Going through the whole range, it does appear to be a bit jumpy?

At about 100 degrees, the padding changes:

At about 2.5 width, the padding changes:

import matplotlib.pyplot as plt
from matplotlib.animation import FuncAnimation
from matplotlib.widgets import Slider

fig, ax = plt.subplots(3, 1, height_ratios=[4, 1, 1])

texts = []
for i, arrow in enumerate(['larrow', 'rarrow', 'darrow']):
    texts.append(
        ax[0].text(0.5, 0.3*i + 0.2, 'Arrow', ha='center', size=16,
                   bbox=dict(boxstyle=f"{arrow}, pad=0.3, head_angle=150")))


def update_head_angle(value):
    for t in texts:
        t.get_bbox_patch().get_boxstyle().head_angle = value
    fig.canvas.draw_idle()


def update_head_width(value):
    for t in texts:
        t.get_bbox_patch().get_boxstyle().head_width = value
    fig.canvas.draw_idle


angle_slider = Slider(ax[1], 'Angle', 0, 360, valinit=150, closedmin=False)
angle_slider.on_changed(update_head_angle)

width_slider = Slider(ax[2], 'Width', 0, 3, valinit=1.5)
width_slider.on_changed(update_head_width)

anim = FuncAnimation(fig, lambda t: angle_slider.set_val(t),
                     frames=range(1, 360), interval=3000/360)
anim.save('angle.gif')

angle_slider.set_val(150)

anim = FuncAnimation(fig, lambda t: width_slider.set_val(t/100),
                     frames=range(0, 300), interval=3000/300)
anim.save('width.gif')

# plt.show()

The discontinuity comes from the fact that for small arrows, we need a margin

whereas for reasonably large arrows we left it out:

The old behavior shifted the text into the arrow (equivalent to a negative margin), but that does not work for various combinations of arrow parameters and detecting that in detail would be much more complex.

Alternatives to the jump would be

1. always add the margin - that felt a bit too much to me but YMMV, and it is even further from the old behavior

2. Make the margin dependent on arrow parameters at the cost of more complex logic.

Discussed an the call. Keeping consistency on these is not super important, no one should be extracting quantitative information from these arrows.

• simplest option (Tim's 1): go to fixed padding
  • pro: dead simple
  • con: not as aesthetic
• smart algorithm (Tim's 2): proposed by @anntzer to adjust the tail such that the arrow just touches the text bounding box
  • pro: might look a bit better
  • con: more complicated, has some odd behavior at extreme aspect ratios and/or big arrows that will have to be thought bout
• just accept the jump
  • pro: it is done! We do not think anyone will actually make the slider Elliott made above and as noted no one should be
  • con: the jump feels wrong, having two magic numbers (the padding value and the change over thresholds) is not great.

Personally I think the smart algorithm will probably look better, but fixed pad is my second choice.

Just as a reminder, I've drawn all(?) the options in #29998 (comment), #29998 (comment).

The bounding-box options (4), (5) are generally not as nice as thought; or at least not better than the simple ones (1), (2). I believe if we want to be smart, we must be even smarter - possibly a combination of cases or some more advanced distance metric for tip-to-text.

Would it be an option to mark the behavior as provisional and allow us the freedom to later change the head positioning algorithm? - Only slight caveat is that we change the behavior for existing users now (one-time break is justified to enable generalization), but we would possibly break them again and leave them in a limbo state in between. Feature flags (#30519) would help with these, but we'd still have to implement them soon.

Overall, possibly the sane decision is to defer this topic to after the 3.11 release.

As a meta comment - if the goal is "to bring attention to a certain region on the axes" this doesn't seem like the right tool. I'd make a precisely placed arrow and add text over top and adjust the text to fit the aesthetics of the arrow. Trying to reverse engineer that behaviour into something precisely depending on a text bounding box doesn't seem like a good API.

That is true and the reason why we can do a breaking change in size/positioning.

The following patch implements the "tangent shaft" rules suggested above. The resulting shafts vary continuously with head width and angle; the only somewhat awkward case is the one of an arrow shaft longer than the box itself (at the beginning of the varying angle example); it could easily be changed to be drawn as a "full triangle" instead if we decide to do so.

diff --git i/lib/matplotlib/patches.py w/lib/matplotlib/patches.py
index 0c8d6b5fee..e7b3b77362 100644
--- i/lib/matplotlib/patches.py
+++ w/lib/matplotlib/patches.py
@@ -2526,7 +2526,7 @@ class BoxStyle(_Style):
     class LArrow:
         """A box in the shape of a left-pointing arrow."""
 
-        def __init__(self, pad=0.3):
+        def __init__(self, pad=0.3, head_width=1.5, head_angle=90):
             """
             Parameters
             ----------
@@ -2534,25 +2534,46 @@ class BoxStyle(_Style):
                 The amount of padding around the original box.
             """
             self.pad = pad
+            if head_width < 0:
+                raise ValueError("'head_width' must be nonnegative")
+            self.head_width = head_width
+            if head_angle % 360 == 0:
+                raise ValueError("'head_angle' must be nonzero")
+            self.head_angle = head_angle
 
         def __call__(self, x0, y0, width, height, mutation_size):
-            # padding
+            # padding & padded dimensions
             pad = mutation_size * self.pad
-            # width and height with padding added.
-            width, height = width + 2 * pad, height + 2 * pad
-            # boundary of the padded box
+            dx, dy = width + 2 * pad, height + 2 * pad
             x0, y0 = x0 - pad, y0 - pad,
-            x1, y1 = x0 + width, y0 + height
+            x1, y1 = x0 + dx, y0 + dy
 
-            dx = (y1 - y0) / 2
-            dxx = dx / 2
-            x0 = x0 + pad / 1.4  # adjust by ~sqrt(2)
+            head_dy = self.head_width * dy
+            mid_y = (y0 + y1) / 2
+            shaft_y0 = mid_y - head_dy / 2
+            shaft_y1 = mid_y + head_dy / 2
 
-            return Path._create_closed(
-                [(x0 + dxx, y0), (x1, y0), (x1, y1), (x0 + dxx, y1),
-                 (x0 + dxx, y1 + dxx), (x0 - dx, y0 + dx),
-                 (x0 + dxx, y0 - dxx),  # arrow
-                 (x0 + dxx, y0)])
+            cot = 1 / math.tan(math.radians(self.head_angle / 2))
+
+            if cot > 0:
+                # tip_x is chosen s.t. the angled line moving back from the tip hits
+                # i) if head_width > 1: the box corner, or ii) if head_width <
+                # 1 the box edge at the point giving the correct shaft width.
+                tip_x = x1 + cot * min(dy, head_dy) / 2
+                shaft_x = tip_x - cot * head_dy / 2
+                return Path._create_closed([
+                    (x0, y0), (shaft_x, y0), (shaft_x, shaft_y0),
+                    (tip_x, mid_y),
+                    (shaft_x, shaft_y1), (shaft_x, y1), (x0, y1),
+                ])
+            else:
+                dx = -cot * (head_dy - dy) / 2  # cot < 0!
+                return Path._create_closed([
+                    (x0 + min(dx, 0), y1), (x0 - max(dx, 0), shaft_y1),
+                    (x0 - max(dx, 0), shaft_y0), (x0 + min(dx, 0), y0),
+                    (x1 - min(dx, 0), y0), (x1 + max(dx, 0), shaft_y0),
+                    (x1 + max(dx, 0), shaft_y1), (x1 - min(dx, 0), y1),
+                ])
 
     @_register_style(_style_list)
     class RArrow(LArrow):
@@ -2565,41 +2586,45 @@ class BoxStyle(_Style):
             return p
 
     @_register_style(_style_list)
-    class DArrow:
+    class DArrow(LArrow):
         """A box in the shape of a two-way arrow."""
-        # Modified from LArrow to add a right arrow to the bbox.
-
-        def __init__(self, pad=0.3):
-            """
-            Parameters
-            ----------
-            pad : float, default: 0.3
-                The amount of padding around the original box.
-            """
-            self.pad = pad
+        # Modified from LArrow to add a right arrow to the bbox; see comments above.
 
         def __call__(self, x0, y0, width, height, mutation_size):
-            # padding
+            # padding & padded dimensions
             pad = mutation_size * self.pad
-            # width and height with padding added.
-            # The width is padded by the arrows, so we don't need to pad it.
-            height = height + 2 * pad
-            # boundary of the padded box
-            x0, y0 = x0 - pad, y0 - pad
-            x1, y1 = x0 + width, y0 + height
+            dx, dy = width + 2 * pad, height + 2 * pad
+            x0, y0 = x0 - pad, y0 - pad,
+            x1, y1 = x0 + dx, y0 + dy
 
-            dx = (y1 - y0) / 2
-            dxx = dx / 2
-            x0 = x0 + pad / 1.4  # adjust by ~sqrt(2)
+            head_dy = self.head_width * dy
+            mid_y = (y0 + y1) / 2
+            shaft_y0 = mid_y - head_dy / 2
+            shaft_y1 = mid_y + head_dy / 2
 
-            return Path._create_closed([
-                (x0 + dxx, y0), (x1, y0),  # bot-segment
-                (x1, y0 - dxx), (x1 + dx + dxx, y0 + dx),
-                (x1, y1 + dxx),  # right-arrow
-                (x1, y1), (x0 + dxx, y1),  # top-segment
-                (x0 + dxx, y1 + dxx), (x0 - dx, y0 + dx),
-                (x0 + dxx, y0 - dxx),  # left-arrow
-                (x0 + dxx, y0)])
+            cot = 1 / math.tan(math.radians(self.head_angle / 2))
+
+            if cot > 0:
+                tip_x0 = x0 - cot * min(dy, head_dy) / 2
+                shaft_x0 = tip_x0 + cot * head_dy / 2
+                tip_x1 = x1 + cot * min(dy, head_dy) / 2
+                shaft_x1 = tip_x1 - cot * head_dy / 2
+                return Path._create_closed([
+                    (shaft_x0, y1), (shaft_x0, shaft_y1),
+                    (tip_x0, mid_y),
+                    (shaft_x0, shaft_y0), (shaft_x0, y0),
+                    (shaft_x1, y0), (shaft_x1, shaft_y0),
+                    (tip_x1, mid_y),
+                    (shaft_x1, shaft_y1), (shaft_x1, y1),
+                ])
+            else:
+                dx = -cot * (head_dy - dy) / 2  # cot < 0!
+                return Path._create_closed([
+                    (x0, y0),
+                    (x1 - min(dx, 0), y0), (x1 + max(dx, 0), shaft_y0),
+                    (x1 + max(dx, 0), shaft_y1), (x1 - min(dx, 0), y1),
+                    (x0, y1),
+                ])
 
     @_register_style(_style_list)
     class Round:

This needs to be rebased appropriately to give credit to the earlier authors (e.g. by committing their tests and docs after proper editing); as usual I'll just post the patch here for now, feel free to pick it up and turn it into a PR if I don't do that first.

This is superseeded by #31198. @CharlieThornton33 thanks for your detailed work, it's included (with attribution) in the follow up.