Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add test for AxisArtist #23534

wants to merge 2 commits into from

Conversation

oscargus
Copy link
Member

@oscargus oscargus commented Aug 1, 2022

PR Summary

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

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

timhoffm
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
Member Author

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 bottom axisline, 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 tacaswell added this to the v3.6.0 milestone Aug 1, 2022
@oscargus oscargus marked this pull request as draft August 12, 2022 14:58
@oscargus
Copy link
Member Author

I swapped to an image test here. However, it seems like AxisArtist 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 in 6383e43

# 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 [a for a in artists if a.get_visible() and a.get_in_layout()
and (isinstance(a, noclip) or not a._fully_clipped_to_axes())]
as AxisArtist 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...

@oscargus
Copy link
Member Author

There is also an issue with the window extent for AxisLabel, 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 QuLogic modified the milestones: v3.6.0, v3.7.0 Sep 9, 2022
@leejjoon
Copy link
Contributor

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.

@leejjoon
Copy link
Contributor

There is also an issue with the window extent for AxisLabel, 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 when get_tightbbox is called. The fix is rather straight forward. I will post a fix soon.

@tacaswell tacaswell modified the milestones: v3.7.0, v3.8.0 Jan 5, 2023
@ksunden ksunden modified the milestones: v3.8.0, future releases Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@timhoffm timhoffm timhoffm left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
status: needs rebase status: needs revision topic: mpl_toolkit topic: testing
Projects
None yet
Milestone
future releases
Development

Successfully merging this pull request may close these issues.
6 participants
@oscargus @leejjoon @QuLogic @timhoffm @tacaswell @ksunden