Annotation: always use FancyBboxPatch instead of bbox_artist #4178

efiring · 2015-03-01T08:07:29Z

This fixes #4139, as an alternative to #4145, by eliminating the use of the bbox_artist debugging function. With a second commit, it also fixes #4140.

efiring · 2015-03-02T02:05:43Z

I'm not entirely happy with the second commit because I still don't understand how, when, and where the transforms are evolving. I determined that prior to this commit the return from patchA.get_transform() being used during clipping and shrinking of the arrow path was incorrect, but it was correct after everything was drawn. I then guessed that drawing the text, along with patchA, would update patchA's transform, and that turned out to be the case.
There are still some problems. First, switching from bbox_artist to a FancyBboxPatch makes a larger box. I suspect (or at least hope) this will be easy to fix. Second, when a YAArrow is used instead of a FancyArrowPatch, it's end is located based on the text bbox, not the FancyBboxPatch boundary, so it tends to extend into the Patch. This is not new.
I haven't looked in the tests directory, but I get the impression that we have quite a body of very complex and hard to understand text/annotation/patch code with very little in the way of automated tests.

tacaswell · 2015-03-02T02:51:42Z

I am regretting both my Annotation refactor from last cycle and merging the bounding box fix into 1.4.x as it seems to have opened a can of worms we were happily ignoring for years.

efiring · 2015-03-02T02:52:08Z

The Travis failure is because the pylab_examples/text_rotation.py is including a "pad" in a dictionary provided as the 4th arg to text(). The way this argument/kwarg is being handled, and described, is confusing, to say the least. Try tracing it through pyplot to axes to Text. Before my change it was being popped off of a copy of a dictionary, and I deleted all that. Evidently a pad argument needs to go back in, presumably to control the padding of the 'square' FancyBboxPatch.

efiring · 2015-03-02T02:54:14Z

@tacaswell, worms probably would have started crawling out sooner or later; but my dislike for working in this part of the code, which I have mostly avoided, has been strongly reinforced.

efiring · 2015-03-02T06:58:46Z

Adjusting the size of the FancyBboxPatch appears to be rather difficult and intrusive. I tried what seemed like a straightforward way to remove some of the extra space below the text--and I failed miserably.
A side effect of dropping the bbox_artist option is that the text box patch now rotates with the text, so pylab_examples/text_rotation.py yields a very different picture. I think that the FBP behavior is what a user would want, so I don't think that losing the old behavior, in which the unrotated extent rectangle is used, will be missed by many people.

tacaswell · 2015-03-12T23:24:18Z

lib/matplotlib/patches.py

@@ -3897,7 +3897,7 @@ def __init__(self, posA=None, posB=None,
        """
        If *posA* and *posB* is given, a path connecting two point are
        created according to the connectionstyle. The path will be
-        clipped with *patchA* and *patchB* and further shirnked by
+        clipped with *patchA* and *patchB* and further shrinked by


efiring · 2015-03-14T23:26:07Z

lib/matplotlib/text.py

        else:
-            self._bbox.update(dict(facecolor=color))
+            self._bbox_patch.update(dict(facecolor=color))


@tacaswell, your comment on this line, and my response, were lost when I rebased. You thought the line should be dedented, but I don't understand why. The update is needed only if a new _bbox_patch has not just been created.

I don't remember exactly what I was thinking when I left the last message. I think what I was suggesting was to get rid of the else block and just always update the _bbox_patch (even if we just created it with the correct color). It is a no-op and I think makes the code path a little easier to follow (which is a style call).

Avoiding else statements and having if blocks which just ensure assumptions are true is is a newish (and possibly not good) style-tick I have developed.

I think my comment on this can be ignored.

tacaswell · 2015-03-15T02:48:59Z

I think this also closes #4012

tacaswell · 2015-03-15T02:51:29Z

Can you add an API changes note?

We probably want to back-port this to color-overhaul as I broke this by merging what I thought were bug-fixes into 1.4.3.

efiring · 2015-03-15T07:43:17Z

This is getting close to ready. @mdboom or @tacaswell, the remaining problem is that the arrow starts inside the patch. The expected clipping by that patch is not working yet.

efiring · 2015-03-15T19:48:58Z

This is just too complicated. I think something is going wrong in the chain of events leading to the clip method of the Base class inside the ConnectionStyle class which is called by FancyArrowPatch. My guess is that some transform is being changed too early, or too late, so that the attempted clipping is working with a bbox_patch and an arrow_patch that are not being transformed the same way. I can't spend any more time on it. @mdboom, will you be able to take a look? Maybe at the same time you will be able to figure out a way to make all this less convoluted and hard to follow. The capabilities provided by all the Fancy things are wonderful, but figuring out how it all works--or, at present, doesn't quite work--is extraordinarily difficult. Maybe all it needs is some overview documentation or additional comments.

efiring · 2015-03-15T20:45:41Z

The Travis failure is because of a subtle change in arrow patch location and size. We can either change the test images or change the patch location and size for these cases. I know how to do either; this is not a problem. In fact, I suspect the aesthetic optimum might be half way between the two cases; previously there was a little too much space below the text, because the full descent was being added. I took that out to get a better match to what the bbox_artist was doing.

tacaswell · 2015-03-16T01:07:12Z

attn @leejjoon. I think you wrote the original version of most of this code, any guidance on where it is going wrong?

efiring · 2015-06-21T02:29:26Z

I rebased this to master, but I'm still struggling with the problem that the YAArrow extends to the text bbox, which in general is inside the fancy patch. I tried setting a direction-reversed fancy patch path as a clip region, hoping this would mean the inside of the polygon would be the blanked-out region, but that didn't work. The solution might be to abandon the YAArrow and simulate it with a FancyArrowPatch. An advantage would be that this would mean that the arrow in Annotation would always be a FancyArrowPatch. The trick would be to make the API and behavior similar enough to the original YAArrow version to keep people happy.

tacaswell · 2015-06-21T03:49:52Z

👍 in principle to dropping YAArrow and just using FanceArrowPatch for
everything. The variability of that code path bothers me.

On Sat, Jun 20, 2015 at 10:29 PM Eric Firing notifications@github.com
wrote:

I rebased this to master, but I'm still struggling with the problem that
the YAArrow extends to the text bbox, which in general is inside the fancy
patch. I tried setting a direction-reversed fancy patch path as a clip
region, hoping this would mean the inside of the polygon would be the
blanked-out region, but that didn't work. The solution might be to abandon
the YAArrow and simulate it with a FancyArrowPatch. An advantage would be
that this would mean that the arrow in Annotation would always be a
FancyArrowPatch. The trick would be to make the API and behavior similar
enough to the original YAArrow version to keep people happy.

—
Reply to this email directly or view it on GitHub
#4178 (comment)
.

efiring · 2015-06-22T04:13:44Z

I think this is getting close to being ready. It won't pass the tests, but we have to figure out where this means the code needs adjustment, and where we need new test images. There is also some more documentation and cleanup to be done. The YAArrow replacement does not exactly reproduce the old behavior and API, but I think it will be close enough, and noticeably improved. It removes the embarrassing aspect of annotation_demo.py in which the short arrow has an absurdly short head.

Some comment and docstring typos are also fixed.

YAArrow is now partly simulated with the FancyArrowPatch, which remains as the only arrow class used by Annotation. I ignored the 'frac' key and added the 'headlength' key; the 'frac' was never a good API because it scaled head length with the arrow length, but left all other dimensions in units of points.

efiring · 2015-07-20T02:00:27Z

I think this is ready to go.

tacaswell · 2015-07-20T15:56:21Z

lib/matplotlib/text.py

            if self.arrow_patch.figure is None and self.figure is not None:
                self.arrow_patch.figure = self.figure
            self.arrow_patch.draw(renderer)

+        # Draw text, including FancyBboxPatch, after FancyArrowPatch.
+        # Otherwise, a wedge arrowstyle can land partly on top of the Bbox.
        Text.draw(self, renderer)


use super here (you use it in update)?

True, I did. That is new code, and right next to another method in which super is used. But the "Text.draw()" is not new, and it is one of several such lines in this module. I'm inclined to think that "super-ifying" the module is out of scope for this PR. I also note that not all parent class method calls in this module can be handled with super; Annotation.__init__ needs to call the __init__ from each of its immediate parents. The only way to avoid that would be to rewrite it so as to use composition rather than multiple inheritance. In the long run that could bring a big gain in readability and maintainability, but given the present complexity of this entire family of code, it would be quite a job.

MNT: always use FancyBboxPatch instead of bbox_artist

This is response to breaking changes (matplotlib/matplotlib#4178) in MPL 1.5.

efiring added the status: needs review label Mar 1, 2015

cimarronm mentioned this pull request Mar 1, 2015

Fixed Annotation draw method #4145

Closed

tacaswell reviewed Mar 12, 2015
View reviewed changes

tacaswell added status: needs revision and removed status: needs review labels Mar 12, 2015

tacaswell added this to the next point release milestone Mar 12, 2015

efiring force-pushed the annotate branch from c7865f0 to e7f3fb7 Compare March 14, 2015 23:10

efiring reviewed Mar 14, 2015
View reviewed changes

tacaswell added status: needs review and removed status: needs revision labels Mar 15, 2015

tacaswell modified the milestones: Color overhaul, next point release Mar 15, 2015

efiring mentioned this pull request Mar 29, 2015

Fixed mispelling of shrinked #4294

Closed

tacaswell mentioned this pull request May 3, 2015

Setting annotation background covers arrow #4400

Closed

efiring force-pushed the annotate branch from bf4bae6 to 25d259c Compare June 20, 2015 22:01

efiring added 7 commits July 19, 2015 09:31

Annotation: always use FancyBboxPatch instead of bbox_artist

9d933bc

Annotate: draw text before arrow; closes matplotlib#4140

7829262

Some comment and docstring typos are also fixed.

Grammer of "shrink" variations

ebaa39a

Correct the default size of text bbox patch and support 'pad' kwarg

c1bc666

remove more YAArrow debris

1e6af77

handle pad defaults; fix sizing of YAArrow substitute

2eb4313

efiring force-pushed the annotate branch from f2aa4b2 to 2eb4313 Compare July 20, 2015 00:04

efiring added 2 commits July 19, 2015 14:21

pep8

3e88b3a

Handle the case in which boxstyle is a callable, not a string

9eceef3

tacaswell reviewed Jul 20, 2015
View reviewed changes

tacaswell modified the milestones: next point release, Color overhaul Jul 20, 2015

tacaswell added a commit that referenced this pull request Jul 22, 2015

Merge pull request #4178 from efiring/annotate

bc185a2

MNT: always use FancyBboxPatch instead of bbox_artist

tacaswell merged commit bc185a2 into matplotlib:master Jul 22, 2015

tacaswell removed the status: needs review label Jul 22, 2015

tacaswell mentioned this pull request Jul 22, 2015

Arrow annotations behave differently between 1.3.1 and 1.4.2 #4012

Closed

efiring deleted the annotate branch July 22, 2015 07:26

efiring mentioned this pull request Jul 22, 2015

Annotation with negative axes fraction coordinate placed incorrectly with v1.4.3 #4292

Closed

tacaswell mentioned this pull request Aug 18, 2015

text update properties does not handle bbox properly #4942

Merged

efiring mentioned this pull request Dec 19, 2016

[WIP] API: remove unused attribute on Annotation #7445

Closed

has2k1 added a commit to has2k1/plotnine that referenced this pull request Apr 25, 2017

Change facet label implementation

39a84db

This is response to breaking changes (matplotlib/matplotlib#4178) in MPL 1.5.

afvincent mentioned this pull request May 10, 2018

MNT: deprecate patches.YAArrow #11213

Merged

6 tasks

timhoffm mentioned this pull request Nov 24, 2021

[Doc]: relpos is not valid for FancyArrowPatch #21732

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotation: always use FancyBboxPatch instead of bbox_artist #4178

Annotation: always use FancyBboxPatch instead of bbox_artist #4178

efiring commented Mar 1, 2015

efiring commented Mar 2, 2015

tacaswell commented Mar 2, 2015

efiring commented Mar 2, 2015

efiring commented Mar 2, 2015

efiring commented Mar 2, 2015

tacaswell Mar 12, 2015

efiring Mar 14, 2015

tacaswell Mar 15, 2015

tacaswell commented Mar 15, 2015

tacaswell commented Mar 15, 2015

efiring commented Mar 15, 2015

efiring commented Mar 15, 2015

efiring commented Mar 15, 2015

tacaswell commented Mar 16, 2015

efiring commented Jun 21, 2015

tacaswell commented Jun 21, 2015

efiring commented Jun 22, 2015

efiring commented Jul 20, 2015

tacaswell Jul 20, 2015

efiring Jul 20, 2015

tacaswell Jul 20, 2015

Annotation: always use FancyBboxPatch instead of bbox_artist #4178

Annotation: always use FancyBboxPatch instead of bbox_artist #4178

Conversation

efiring commented Mar 1, 2015

efiring commented Mar 2, 2015

tacaswell commented Mar 2, 2015

efiring commented Mar 2, 2015

efiring commented Mar 2, 2015

efiring commented Mar 2, 2015

tacaswell Mar 12, 2015

Choose a reason for hiding this comment

efiring Mar 14, 2015

Choose a reason for hiding this comment

tacaswell Mar 15, 2015

Choose a reason for hiding this comment

tacaswell commented Mar 15, 2015

tacaswell commented Mar 15, 2015

efiring commented Mar 15, 2015

efiring commented Mar 15, 2015

efiring commented Mar 15, 2015

tacaswell commented Mar 16, 2015

efiring commented Jun 21, 2015

tacaswell commented Jun 21, 2015

efiring commented Jun 22, 2015

efiring commented Jul 20, 2015

tacaswell Jul 20, 2015

Choose a reason for hiding this comment

efiring Jul 20, 2015

Choose a reason for hiding this comment

tacaswell Jul 20, 2015

Choose a reason for hiding this comment