Fixed Annotation draw method #4145

cimarronm · 2015-02-22T19:21:52Z

Fix for issue #4139

This correctly draws the text portion of the annotation after incorporation of PR #4023.

Before

After

…f the annotation

efiring · 2015-02-22T20:15:28Z

lib/matplotlib/text.py

@@ -2092,7 +2093,9 @@ def draw(self, renderer):
                self.arrow_patch.figure = self.figure
            self.arrow_patch.draw(renderer)

-        Text.draw(self, renderer)
+        astext = copy.copy(self)
+        astext.__class__ = Text


Evidently it works, but this seems like a hack. Maybe what it is telling us is that the Annotation could be implemented more cleanly via composition than via inheritance. That's speculation; I haven't looked closely.

Yes, I agree, while going through the Text module, Annotation does seem like it might be better suited for composition rather than inheritance. There is some precedence for modifying the __class__ attribute in the codebase currently but ultimately it is a hack to get the desired MRO.

One alternative approach is to refactor the Text.draw method to not reuse bbox_artist but that just seems like a step in the wrong direction by duplicating code. Another is to design a Text initialization that takes the derived class and returns a Text instance.

I think the problem is with bbox_artist() from patch.py, which adds a patch. It calls the object's get_window_extent(). For Annotations, that includes the arrow.

Note that by "problem" I don't mean that bbox_artist() is broken, just that some assumption somewhere is broken at that point.

@WeatherGod Yes, that is correct (see my comment on issue #4139).

Well, if that is the case, then I am -1 on this kludge. We know exactly
what the problem is -- we shouldn't be using bbox_artist() for this.
Perhaps "bbox_text()"? It would work similarly to bbox_artist, but would
use the Text class's get_window_extent no matter what object is passed to
it?

As for precedence for explicitly changing an object's class, I am
assuming that you are referring to mplot3d. Yeah, let's not bring that wart
into matplotlib proper. I still haven't figured out a way to fix that.

On Mon, Feb 23, 2015 at 4:58 PM, Cimarron notifications@github.com wrote:

In lib/matplotlib/text.py
#4145 (comment):

@@ -2092,7 +2093,9 @@ def draw(self, renderer):
self.arrow_patch.figure = self.figure
self.arrow_patch.draw(renderer)

Text.draw(self, renderer)

astext = copy.copy(self)

astext.**class** = Text

@WeatherGod https://github.com/WeatherGod Yes, that is correct (see my
comment on issue #4139
#4139).

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4145/files#r25206783.

tacaswell · 2015-02-22T20:57:10Z

Currently Annotation is implemented via multiple inheritance (class Annotation(Text, _AnnotationBase)) with the annotation details being a mix-in on top of the Text class. Pulling this apart for composition sounds good, but that will break all manner of back compatibility with code that expects to get a Text object back from annotation calls.

efiring · 2015-02-22T21:13:34Z

Right. Given that we are stuck with the present implementation, it looks like this PR is the simplest fix for the bug.

tacaswell · 2015-02-24T03:02:17Z

def bbox_artist(artist, renderer, props=None, fill=True):
    """
    This is a debug function to draw a rectangle around the bounding
    box returned by
    :meth:`~matplotlib.artist.Artist.get_window_extent` of an artist,
    to test whether the artist is returning the correct bbox.

    *props* is a dict of rectangle props with the additional property
    'pad' that sets the padding around the bbox in points.
    """

A bigger problem seems to be that we are using what is labeled as a debugging function in production code. It also looks like it results in the creation of a new artist during the draw which gets drawn to the canvas, but is not a child of the Axes which also seems really odd to me.

We probably should figure out a cleaner way to implement this (maybe fully adopting the 'fancy' way or adding a rectangle artist to the Text object as a child?).

This is the sort of thing that will make serialization/export to other libraries 'interesting'...

tacaswell · 2015-02-24T03:06:52Z

On the other hand, it looks like that bit of code dates from JDH from 2004 (e50ff69).

WeatherGod · 2015-02-24T03:35:13Z

For all we know, it may have started out as a debugging feature. I know I
would have probably done something like that as a way to make sure all the
bounding boxes are what I expect them to be.

You are absolutely right, I didn't even think about the problem of adding
yet another Artist during the draw(). That seems to be really awkward, but
was probably needed back when there was no way to guarantee a proper bbox
calculation until after the initial text draw. Aren't we a little better
about that now?

On Mon, Feb 23, 2015 at 10:06 PM, Thomas A Caswell <notifications@github.com

wrote:

On the other hand, it looks like that bit of code dates from JDH from 2004
(e50ff69
e50ff69
).

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

efiring · 2015-03-01T01:07:49Z

bbox_artist is used purely in a debug mode in offsetbox.py and axis.py; this one location in text.py is the only place it is used in a production mode.
One solution would be to eliminate the use of bbox_artist in Text by always using the FancyBboxPatch instead. This would considerably simplify the code in Text. The only references to the path leading to bbox_artist are via a private attribute, _bbox, so I don't see a compatibility problem. This would also be the opportunity to rectify an inconsistency in that set_background_color presently operates only on _bbox, not _bbox_patch. As I noted on #4139, a clipping problem will remain to be solved--but I suspect that will be straightforward.

WeatherGod · 2015-03-01T01:37:56Z

I am +1 on that approach.
On Feb 28, 2015 8:07 PM, "Eric Firing" notifications@github.com wrote:

bbox_artist is used purely in a debug mode in offsetbox.py and axis.py;
this one location in text.py is the only place it is used in a production
mode.
One solution would be to eliminate the use of bbox_artist in Text by
always using the FancyBboxPatch instead. This would considerably simplify
the code in Text. The only references to the path leading to bbox_artist
are via a private attribute, _bbox, so I don't see a compatibility
problem. This would also be the opportunity to rectify an inconsistency in
that set_background_color presently operates only on _bbox, not
_bbox_patch. As I noted on #4139
#4139, a clipping
problem will remain to be solved--but I suspect that will be
straightforward.

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

cimarronm · 2015-03-01T19:40:17Z

Closing in favor or #4178

BUG: Fixed annotation draw method to properly draw the text portion o…

973831f

…f the annotation

tacaswell added the status: needs review label Feb 22, 2015

efiring reviewed Feb 22, 2015
View reviewed changes

efiring mentioned this pull request Mar 1, 2015

Annotation: always use FancyBboxPatch instead of bbox_artist #4178

Merged

cimarronm closed this Mar 1, 2015

tacaswell removed the status: needs review label Mar 1, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Annotation draw method #4145

Fixed Annotation draw method #4145

cimarronm commented Feb 22, 2015

efiring Feb 22, 2015

cimarronm Feb 22, 2015

WeatherGod Feb 23, 2015

WeatherGod Feb 23, 2015

cimarronm Feb 23, 2015

WeatherGod Feb 24, 2015

tacaswell commented Feb 22, 2015

efiring commented Feb 22, 2015

tacaswell commented Feb 24, 2015

tacaswell commented Feb 24, 2015

WeatherGod commented Feb 24, 2015

efiring commented Mar 1, 2015

WeatherGod commented Mar 1, 2015

cimarronm commented Mar 1, 2015

Fixed Annotation draw method #4145

Fixed Annotation draw method #4145

Conversation

cimarronm commented Feb 22, 2015

Before

After

efiring Feb 22, 2015

Choose a reason for hiding this comment

cimarronm Feb 22, 2015

Choose a reason for hiding this comment

WeatherGod Feb 23, 2015

Choose a reason for hiding this comment

WeatherGod Feb 23, 2015

Choose a reason for hiding this comment

cimarronm Feb 23, 2015

Choose a reason for hiding this comment

WeatherGod Feb 24, 2015

Choose a reason for hiding this comment

tacaswell commented Feb 22, 2015

efiring commented Feb 22, 2015

tacaswell commented Feb 24, 2015

tacaswell commented Feb 24, 2015

WeatherGod commented Feb 24, 2015

efiring commented Mar 1, 2015

WeatherGod commented Mar 1, 2015

cimarronm commented Mar 1, 2015