I don't think visibility affects the extents of any of the artists? Its up to whatever uses the extents to decide whether to include them or not. I actually don't think the current code is correct - it should just return the extent.

I don't think visibility affects the extents of any of the artists? Its up to whatever uses the extents to decide whether to include them or not. I actually don't think the current code is correct - it should just return the extent.

Okay, but it affectstight_layout, and if an artist doesn't even exist in the output PDF/SVG, why should its bounds be used to calculate the layout?

I'm fine if you tell me thatget_window_extents should ignore whether the artist exists or not, but then do you agree this changeshould move intoget_tightbbox at least?

You can easily argue the converse that the user put the empty thing out there for a reason, and we should respect it. i.e. imagine they are toggling the text on and off, but want the plot size to stay the same.

I think all I'm suggesting is that we should be careful about changing this, and to make every effort to make it consistent with other artists.

You can easily argue the converse that the user put the empty thing out there for a reason, and we should respect it. i.e. imagine they are toggling the text on and off, but want the plot size to stay the same.

But this will not cause the plot to stay the same size (unless the text is on the top left of the plot, andva='top', ca='left', or similarly for the other four cases). Otherwise as the text shrinks or grows, the page size will still change. I can see why visibility should not affect the bbox calculation now, so I'll revert that part of the PR. I'll even grant that reverting the current empty stringText behavior (that it acts like a single pixel at thexy location specified for theText) would probably trample on a lot of existing code. But I'm not convinced that this is a use case that it would actually mess with.

I think all I'm suggesting is that we should be careful about changing this, and to make every effort to make it consistent with other artists.

I see your point and will update the PR momentarily to more carefully address the issue which (in my mind, at least) seems more definitely like a bug...(the case ofannotation_clip). I'm going to assume that the best way to slot this fix into our existing "get size" stack of (get_tightbbox -> get_window_extents) is to implementAnnotation.get_tightbbox since it has a "clip box" thatArtist.get_tightbbox has no reason to know about. In addition, I see now thatget_window_extents is not allowed to returnBbox.null, so actually we needget_tightbbox if we want to indicate a fully-clip'd artist anyway.

Update:@jklymak. You're right that most artists don't seem to change theirget_window_extents based on theirvisibility. However, text already does.

Arguably, this should not be the case, since other figures don't do this. But I'd want other people's opinions on whether to change this, because changing this either way would be a major change totight_layout.

For now, I'll just leave it as-is, and implementtightbbox to correctly ignore the clipped case, similarly to how it's being done elsewhere.

Update Update: Putting this logic intoget_tightbbox instead breaks a ton of tests, so I'll have to come back to this later, but the current PR is still ready for review I suppose, if anybody thinks it should just go in as-is.

The relevant code is here:

So it seems you are saying that the clipping extent is not correct for an Annotation? Thats seems to be the underlying bug, otherwise the clipping should have worked to exclude this artist from the bbox calc.

So as far as I can see, Annotations don't define their clipping extent. In which case you can't really expect them to be clipped. The proper solution is to implement clipping for them.

So as far as I can see, Annotations don't define their clipping extent. In which case you can't really expect them to be clipped. The proper solution is to implement clipping for them.

This was also my first response, however, as I read into the logic of the code further I changed my own mind. Tell me if the following makes sense.

So it seems you are saying that the clipping extent is not correct for an Annotation? Thats seems to be the underlying bug, otherwise the clipping should have worked to exclude this artist from the bbox calc.

I think the clipping extentsare correct for anAnnotation, but it's a little convoluted. Despite its name,annotation_clip appears (to my untrained eye) to have different semantics than other clipping boxes we use elsewhere. Here, it's not an actual clip box (like in the sense typically meant in e.g. PDFs or SVGs), but instead, it's a condition that the user can use to determine whether or not the annotation will be drawn at all based on the presence of a particular point indata space within the axes.

For example, adding logic that includes the axes box as an extra "clip" box for theAnnotation, won't work because theAnnotationis allowed to spill outside the axes, as long as the point it's labeling is within the axes limits in data space.

The actual logic for whether theAnnotation is displayed or not is here in the draw call:

which hands off to this helper function, which enforces the special "do we show this annotation" logic required by theannotation_clip kwarg:

whereas in theget_window_extents call, this same helper function is not checked (althoughget_visible is, which, I agree with you, is inconsistent with elsewhere in the library):

So when the value returned fromget_window_extent gets back up to the line you've referenced, you're right that the clip box is not set. But that (afaict) is correct, since there is no clip box, theAnnotation is being excluded from the draw tree by a different type of logic (but I may be wrong, and obviously onecould implement this as a conditional clip box which is only there in the case when the annotated point is not in the Axes limits, this just felt wrong to me from reading the code).

I initially (incorrectly) thought this logic belonged inAnnotation.get_window_extent since that's where theget_visible check was, but after you pointed out that's not uniform, and I did more digging, my new impression is that the fix should instead be to return the "true" window extents of the (maybe invisible) Artist, as is done everywhere else, but then simply make sure the Artist isn't being excluded from the draw tree before including it in thebbox calculation.

So my new PR would be something like just the following patch:

intext.py, Line 1970:

defget_tightbbox(self,renderer):# docstring inheritedifnotself._check_xy(renderer):returnBbox.null()returnsuper().get_tightbbox(renderer)

however, this (expectedly) leads to hellaNaN bboxes as-is (because of the tragic semantics of Bbox's), so I'd have to clean it up so it interacts as expected with the various functions which request this bbox. <-- actually this might just be my dev environment, I've pushed the newer version to see what Travis says.

I'll block, just because one pixel is better than no pixels ;-)

Okay, I solved the underlying issue, while leaving the existing behavior ofText having a one pixel wideget_window_extents whenever it's not visible.

This change now only affects annotations withannotation_clip=True orNone.

Arguably,get_window_extents should also be changed so that instead of one pixel, the full extents are returned even whennot annotation.get_visible(), but I'll leave that for a future PR.

.. I dropped the release critical flag - this has been like this forever, right?

.. I dropped the release critical flag - this has been like this forever, right?

Yes it has. I tagged it this way because it was technically a "bug", but of course will go with your judgement.

... one persons "Critical" is different depending on need. FWIW< you can always manually exclude any artist usingartist.set_in_layout(False)

... one persons "Critical" is different depending on need. FWIW< you can always manually exclude any artist usingartist.set_in_layout(False)

Fair. I mean for my purposes I can just require this branch/master for now. Added API changes.

I agree with@jklymak on the use of critical for this, we tend to save that for things like seg-faults, egregious regressions, or bugs that make plotswrong. Things that make the libraryunusable rather than inconvenient.