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 affects tight_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 that get_window_extents should ignore whether the artist exists or not, but then do you agree this change should move into get_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, and va='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 string Text behavior (that it acts like a single pixel at the xy location specified for the Text) 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 of annotation_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 implement Annotation.get_tightbbox since it has a "clip box" that Artist.get_tightbbox has no reason to know about. In addition, I see now that get_window_extents is not allowed to return Bbox.null, so actually we need get_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 their get_window_extents based on their visibility. 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 to tight_layout.

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

Update Update: Putting this logic into get_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 extents are correct for an Annotation, 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 in data space within the axes.

For example, adding logic that includes the axes box as an extra "clip" box for the Annotation, won't work because the Annotation is 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 the Annotation 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 the annotation_clip kwarg:

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

So when the value returned from get_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, the Annotation is being excluded from the draw tree by a different type of logic (but I may be wrong, and obviously one could 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 in Annotation.get_window_extent since that's where the get_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 the bbox calculation.

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

in text.py, Line 1970:

    def get_tightbbox(self, renderer):
        # docstring inherited
        if not self._check_xy(renderer):
            return Bbox.null()
        return super().get_tightbbox(renderer)

however, this (expectedly) leads to hella NaN 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 of Text having a one pixel wide get_window_extents whenever it's not visible.

This change now only affects annotations with annotation_clip=True or None.

Arguably, get_window_extents should also be changed so that instead of one pixel, the full extents are returned even when not 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 using artist.set_in_layout(False)

... one persons "Critical" is different depending on need. FWIW< you can always manually exclude any artist using artist.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 plots wrong. Things that make the library unusable rather than inconvenient.