Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MNT: only account for Artists once in fig.get_tightbbox #26899

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Sep 23, 2023

PR summary

Figure.get_tightbbox takes the union of the tightbbox's of all the artists attached to it. This includes each axes, whose tight_bbox incorporates all of their artists:

if bbox_artists is None:
bbox_artists = self.get_default_bbox_extra_artists()
for a in bbox_artists:
bbox = a.get_tightbbox(renderer)
if (bbox is not None
and 0 < bbox.width < np.inf
and 0 < bbox.height < np.inf):
bb.append(bbox)
return mtransforms.Bbox.union(
[b for b in bb if b.width != 0 or b.height != 0])

So it seems redundant to me to also add in the axes' artists individually. Removing these lines did not break any tests for me locally.

PR checklist

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this get a small test since this method is public anyways. I also wonder if this needs an API change note just in case people were somehow using this to get all artists on a figure?

@rcomer
Copy link
Member Author

rcomer commented Sep 23, 2023

Actually we can also leave the axes out from the list of artists as they are accounted for further down the Figure.get_tightbbox method:

for ax in self.axes:
if ax.get_visible():
# some axes don't take the bbox_extra_artists kwarg so we
# need this conditional....
try:
bbox = ax.get_tightbbox(
renderer, bbox_extra_artists=bbox_extra_artists)
except TypeError:
bbox = ax.get_tightbbox(renderer)
bb.append(bbox)

@jklymak yep, I was wondering about the api change. Since the method is now just one list comprehension, we could put that list comprehension directly in get_tightbbox and deprecate get_default_bbox_extra_artists?

@jklymak
Copy link
Member

jklymak commented Sep 24, 2023

I don't know if anyone really uses it this, but you could imagine passing a list of extra artists to get_tightbbox that is made from the list returned by get_default_bbox_extra_artists. Indeed maybe this is a reason to unpack the artists in the axes? Otherwise it's hard to add/remove artists from the list if they are down in the axes.

The set_in_layout property obviates the need for messing with these lists, and I think is a better solution to excluding artists - I don't know what would happen if we just got rid of them.

@rcomer
Copy link
Member Author

rcomer commented Sep 24, 2023

I believe I have evidence of a measurable performance gain here #26150 (comment).

@jklymak
Copy link
Member

jklymak commented Sep 24, 2023

I think we should maybe leave the public method alone because it is a way to get a flat list of all artists you may want to query on a Figure. It could be better named for a more general purpose, but you could imagine folks are using it

However, internally, we should probably stop using this method in favour of a hierarchy, as proposed here.

@rcomer
Copy link
Member Author

rcomer commented Sep 25, 2023

OK, I've removed the deprecation. What happens to the helper method clearly no longer affects the purpose of this PR. If someone has an opinion what to do with it (rename, deprecate,...) it could always be done separately.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still makes sense to me.

@QuLogic QuLogic merged commit 23b51bf into matplotlib:main Sep 26, 2023
40 checks passed
@rcomer rcomer deleted the tight_bbox-redundancy branch September 26, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants