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.

Given that changes are relatively small and that positioning is happening for the texts, not the box tips of the arrow, I'd claim that the box style is rather decoration and not primarily meant to position the tip of the arrow at a specific coordinate. Therefore, I'd be ok with a breaking change if it opens the route for more functionality.

There seems to be some issues with text positioning; for example, we have things like

which comes from the result of test_boxarrow_adjustment.

I'm not sure why this is happening; using print(<text object>.get_bbox_patch().get_path()) on a text object with this bounding box to check the vertices that are being drawn, they seem to be correct (i.e. symmetrically-positioned as they were before 2ac0bff). I'd appreciate any help with this - I can't tell why this issue seems to only be showing up now.

@timhoffm I'm sorry I've not been able to come back to this since early August - I've been really busy with work. I was just wondering whether you've seen any positioning issues like this before; I haven't been able to determine what's different with how the arrow's being drawn now that positions the text differently. Any help would be greatly appreciated.

No worries. It takes as long as it takes. We all have other priorities beyond matplotlib in our lifes.

As to the text positioning, no I haven't seen anything like this before. You could try a variant of this to draw a rectangle bounding box in addtion to the text for debugging purposes https://github.com/matplotlib/matplotlib/pull/29872/files

Interestingly, when you look at boxarrow_adjustment_test_image.png it only happens for darrow, not for larrow and rarrow, so I suspect it's somewhere in the darrow logic not in the text bounding box itself.

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.