Skip to content

[Bug]: possible regression in axis ticks handling in matplotlib 3.6.0rc2 #23806

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

Closed
neutrinoceros opened this issue Sep 5, 2022 · 7 comments
Closed

Comments

@neutrinoceros
Copy link
Contributor

neutrinoceros commented Sep 5, 2022

Bug summary

While running image tests for yt, I noticed a change of behaviour that bisects to
2357c92d87d96d519c8470776e76180e71663d0b (PR #22587)
and that I'm not 100% sure is intentional, where parameters set with Axis.tick_params are forgotten after a call to Axes.cla.

In yt we use persistent Axis instances inside container classes; we typically set some tick_params during instantiation of the container, and defer rendering as much as possible, at which point Axis instances may be cleared before the plot is drawn again.

I think this can easily be fixed in yt, but I wanted to check with you wether this change was intented or not.

Code for reproduction

import matplotlib as mpl
import matplotlib.pyplot as plt

fig, ax = plt.subplots()
ax.tick_params(which="both", axis="both", direction="in", top=True, right=True)
ax.cla()
fig.savefig(f"/tmp/{mpl.__version__}.png")

Actual outcome

3 6 0 dev3625+g25b39d41d3

Expected outcome

with matplotlib 3.5
3 5 0rc2 dev89+g6e95e7bf54

Additional information

No response

Operating system

N/A

Matplotlib Version

3.6 VS 3.5

Matplotlib Backend

MacOSX (but also seeing this on a Linux CI server)

Python version

3.10.4

Jupyter version

N/A

Installation

pip

@oscargus
Copy link
Member

oscargus commented Sep 6, 2022

cc @timhoffm as the author of #22587 (and API lead).

@timhoffm
Copy link
Member

timhoffm commented Sep 6, 2022

Thanks for bringing this up. I think we need a discussion what we mean by clear().

I don't have the full history present and think we might have been inconsistent here.

From a conceptual point of view. IMHO clear() should reset all stylistic attributes (including tickdir, which feels the same category as linewdith to me). #22587 is a rewrite to explicitly not reset tick and ticklabel visibility. This is because visibility is more structural than stylistic. It's e.g. set by shared layouts and resetting visibility would make clear() unusable for shared Axes. Whether that's the clean way would be a separate discussion, but since we don't have a persistent concept of layout connected to tick visiblity, not resetting visibility is the only viable approach. I don't think these arguments hold for other tick properties.

To be checked: We might have been inconsistent and even changed behavior unintendedly or at least without warning. So independent on what is a good behavior, we need to check the past and current behavior and also make sure we have consistency, or if that is not possible at least a documented behavior over time.

@tacaswell tacaswell added this to the v3.6.0 milestone Sep 6, 2022
@jklymak
Copy link
Member

jklymak commented Sep 6, 2022

My intuition is that clearing the axes removes artists, puts the axes back to its default axes limits, and clears label. Ie all data added to the axes would be removed.

OTOH, axes styling - tickdir, tick length, which spines are shown, spine styling, etc would not be reset as these are independent of the data on the axes. That is what I believe the behaviour of Matlab is, and I think it is the behaviour Matplotlib has had until recently.

@neutrinoceros
Copy link
Contributor Author

Note that I opened #23808 to restore the behaviour of matplotlib 3.5, if you guys want to stay conservative.

@tacaswell
Copy link
Member

I'm inclined to be conservative and go back to the 3.5 behavior rather than adding a behavior change note at this point.

I think it is better be consistent through time but inconsistent through the API (unless we can make the API fully consistent in one go or at least have a clearly articulated plan of where we are going).

@timhoffm
Copy link
Member

timhoffm commented Sep 7, 2022

I'm inclined to be conservative and go back to the 3.5 behavior rather than adding a behavior change note at this point.

Agreed, but still we need to consider two which properties this should apply. For example, 3.5 also keeps tick_params(... width=10) under clear() whereas 3.6rc does not. I suppose this holds for a lot of the tick_params.

We should add a figure comparison test

params = {direction='in', width=5, ...[lots of other params to persist]}
fig_test.tick_params(**params)
fig_test.clear()
fig_ref.tick_params(**params)

to make sure we are don't have an unintended regression through the clear refactoring.

@QuLogic
Copy link
Member

QuLogic commented Sep 9, 2022

This was fixed by #23834; discussion for what to do long term is in #23839.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants