NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

Add test for AxisArtist#23534

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Draft

oscargus wants to merge2 commits intomatplotlib:main

base:main

Choose a base branch

fromoscargus:getwindowextent

Draft

Add test for AxisArtist#23534

oscargus wants to merge2 commits intomatplotlib:mainfromoscargus:getwindowextent

+20 −2

Conversation

Copy link

Member

PR Summary

PR Checklist

Tests and Styling

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

oscargus added topic: mpl_toolkit topic: testing labels

Aug 1, 2022

timhoffm reviewed

Aug 1, 2022

View reviewed changes

lib/mpl_toolkits/tests/test_axisartist_axis_artist.py Outdated

Comment on lines 104 to 105

		assert math.isclose(tightbbox.height, 15.880208333333329)
		assert math.isclose(tightbbox.width, 518.0625)

Copy link

Member

Choose a reason for hiding this comment

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

Can these values be understood or do you just pin the current values?

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

The latter. Although I guess one can probably figure them out.

Looking at similar tests there is the whole range from pinned values to computed to relative changes.

Copy link

Member

Choose a reason for hiding this comment

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

As this is the bottomaxisline, I think, then the width should be ~= the Axes width? Height is probably something like text (TTT) height + pad + linewidth, but that's a bit more difficult to put in.

Copy link

Member

Choose a reason for hiding this comment

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

I'm ok pinning the current numbers as regression tests. But please add a comment on this so that we don't have to wonder later where these numbers came from.

tacaswell added this to thev3.6.0 milestone

Aug 1, 2022

QuLogic added the status: needs revision label

Aug 10, 2022

oscargus marked this pull request as draft

August 12, 2022 14:58

oscargus force-pushed thegetwindowextent branch from29c520a to2500c34Compare

August 12, 2022 15:12

oscargus added2 commits

August 12, 2022 17:17

Add test for tight AxisArtist

8631c31

Remove unwanted characters

657e268

oscargus force-pushed thegetwindowextent branch from2500c34 to657e268Compare

August 12, 2022 15:18

Copy link

MemberAuthor

oscargus commentedAug 14, 2022

I swapped to an image test here. However, it seems likeAxisArtist is not really taken into consideration when determining the tight bounding box. It is removed here:

matplotlib/lib/matplotlib/axes/_base.py

Lines 4326 to 4332 in6383e43

	# always include types that do not internally implement clipping
	# to Axes. may have clip_on set to True and clip_box equivalent
	# to ax.bbox but then ignore these properties during draws.
	noclip= (_AxesBase,maxis.Axis,
	offsetbox.AnnotationBbox,offsetbox.OffsetBox)
	return [aforainartistsifa.get_visible()anda.get_in_layout()
	and (isinstance(a,noclip)ornota._fully_clipped_to_axes())]

asAxisArtist is not in the tuple.

This leads me to think that there is something missing here. Probably, rather than having an explicit list, which I assume should not include classes from mpl_toolkits, it may be better to have some property in the class? This will also allow user-defined classes to define themself to always be included here. (It is of course possible to redefine_fully_clipped_to_axes to return False, but that doesn't really look like a solution to encourage...

Copy link

MemberAuthor

oscargus commentedAug 14, 2022

There is also an issue with the window extent forAxisLabel, so not sure if it is worth spending more time on getting this to work?

Should I open an issue with it or just drop it?

QuLogic modified the milestones:v3.6.0,v3.7.0

Sep 9, 2022

github-actionsbot added the status: needs rebase label

Nov 12, 2022

Copy link

Contributor

leejjoon commentedNov 21, 2022

This leads me to think that there is something missing here. Probably, rather than having an explicit list, which I assume should not include classes from mpl_toolkits, it may be better to have some property in the class? This will also allow user-defined classes to define themself to always be included here. (It is of course possible to redefine_fully_clipped_to_axes to return False, but that doesn't really look like a solution to encourage...

At least for the AxisArtist, I think having it to override_fully_clipped_to_axes is a reasonable way. The culprit here is that while AxisArtist itself is clipped to the axes, its children (ticks, ticklables and axislabel) may be not. So, it should check the return values of_fully_clipped_to_axes for its children and return True if any of them is True.

Copy link

Contributor

leejjoon commentedNov 21, 2022

There is also an issue with the window extent forAxisLabel, so not sure if it is worth spending more time on getting this to work?
Should I open an issue with it or just drop it?

This was because the location of ticklabels may not be correctly updated at the time whenget_tightbbox is called. The fix is rather straight forward. I will post a fix soon.