BUG: fix a regression where tick direction wasn't conserved by Axis.clear() #23808

neutrinoceros · 2022-09-05T20:02:25Z

PR Summary

This is a quick fix for #23806, assuming the change in behaviour I reported there wasn't intentional

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).

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

…lear()

jklymak

Will hold off merging if @timhoffm wants to argue this is wrong, unless we are pushing 3.6 final out the door imminently...

timhoffm · 2022-09-07T08:22:19Z

It's ok to go back to 3.5 behavior for this. But we need to consider other parameters as well. 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.

neutrinoceros · 2022-09-07T08:28:17Z

@timhoffm I'm happy to extend the scope of my PR as needed. Should I parametrise my test or extend it so it covers many parameters at once ?

edit: never mind I see that you already answered my question in the issue, I'll work on this now

…p minor)

neutrinoceros · 2022-09-07T08:57:18Z

@timhoffm done

timhoffm

I'm sorry this is such a mess. Let me at least recap the history:

KeyError: 'gridOn' in axis.py when axis.tick_params() is used with reset = True #20149 is the original issue, that started everything
Fix resetting grid visibility #20161 was the bugfix for above (v3.4.2)
resulting in regressions ax.clear() adds extra ticks, un-hides shared-axis tick labels #20721 and Regression: undocumented change of behaviour in mpl 3.4.2 with axis ticks direction #20219.
these were hot-fixed in Fix clear of Axes that are shared. #20826 (v3.4.3)
Refactor handling of tick and ticklabel visibility in Axis.clear() #22587 (v3.6.0) claims to be a follow-up cleanup which claims to not change the outcome, but apparently we did not have enough tests.

The target should be preserving the 3.4.3/3.5.x state for. Maybe we should back out #22587 for 3.6 and defer to 3.7. It's only supposed to be a code cleanup after all. Otherwise, I think we need even better tests to ensure we're doing consistent and right things.

lib/matplotlib/tests/test_axes.py

timhoffm · 2022-09-07T23:40:40Z

lib/matplotlib/tests/test_axes.py

+
+    ax = fig_ref.subplots(1, 1)
+    ax.tick_params(**params)
+    ax.imshow(img)


Is there an advantage in imshow()? I would have expected that an empty plot would work just as well. - Advantage: We save 4 unneeded lines of code and the resulting plot is less cluttered.

That's for testing zorder. If the plot is otherwise empty we can't detect when it's broken. I'll add a comment to clarify this

come to think of it:

I actually forgot to actually test zorder

I doesn't combine nicely with other parameters I'm testing

I'll add a separate test and simplify this one

Actually I don't think I understand what zorder=-1 is supposed to achieve, assuming the behaviour in matplotlib 3.5.3 isn't buggy. I would expect that combining zorder=-1, direction='in' with an image would effectively hide ticks behind the image, but it's not the case. In order to keep this PR on the conservative side, I'm not going to change the behaviour here unless required.

I would expect that combining zorder=-1, direction='in' with an image would effectively hide ticks behind the image, but it's not the case

I would share that expectation

In order to keep this PR on the conservative side, I'm not going to change the behaviour here

👍

timhoffm · 2022-09-08T00:20:02Z

lib/matplotlib/axis.py

@@ -846,6 +846,13 @@ def get_children(self):
        return [self.label, self.offsetText,
                *self.get_major_ticks(), *self.get_minor_ticks()]

+    # style parameters that should be preserved by reset operations


This looks better, but I'm a bit of a loss what we actually want.

What's the policy for e.g.

zorder

the grid parameters like grid_color

We should have an explicit statement on why we keep some parameters but not others.

I intentionally left out grid params because I have no opinion there, but I agree it should be discussed.

zorder seemed to fit in the "visibility" theme, for which a consensus appears to have been reached. Please correct me if I am wrong.

neutrinoceros · 2022-09-08T07:57:56Z

Otherwise, I think we need even better tests to ensure we're doing consistent and right things.

I'm all for that, but could you clarify what you have in mind ?

timhoffm · 2022-09-08T13:18:06Z

Otherwise, I think we need even better tests to ensure we're doing consistent and right things.

I'm all for that, but could you clarify what you have in mind ?

Essentially covering all parameters and making sure that the behavior is consistent before and after #22587 (or 3.5.x and main); i.e. all parameters that we're cleared before should be cleared now, and all parameters that were kept before should be kept now. An additional annoyance to that is that we accept more paramters in tick_params() than documented. The documented ones are translated to tick properties, but we also accept tick properties directly.

neutrinoceros · 2022-09-08T13:52:41Z

I absolutely agree with you, I'm just not sure I'll have the time to dig much further into this. I may, but in the mean time, please feel free to add to this PR if you feel that's appropriate.

timhoffm · 2022-09-08T21:39:47Z

The dev call today we have decided to revert the last change. This will continue the behavior for 3.5 to 3.6. For 3.7 we plan a discussion on what clear() actually should do.

@neutrinoceros thanks for this PR, I'll mark it as draft as it needs some changes after the rebase, but the tests will continue to be usful. Until we had the discussion there is not much you can do in this PR.

neutrinoceros · 2022-09-09T05:37:28Z

Thank you for your work, I really appreciate it !
If I understand correctly this PR should be removed from the 3.6 milestone 🙂

neutrinoceros · 2022-10-18T17:15:36Z

Do you guys wish to keep this open until the discussion is had ? I'm thinking maybe the test could be merged by itself ? I'm fine either way, I'm just trying to cleanup my personal backlog.

tacaswell · 2022-12-16T20:53:49Z

This is unfortunately still in limbo.

neutrinoceros · 2023-10-04T12:36:39Z

It does not seem necessary anymore, so I'll close this. If it ever changes, anyone is free to continue this work and take off where I left.

neutrinoceros force-pushed the hotfix_23806 branch from 160219b to 0f92ac5 Compare September 5, 2022 20:11

github-actions bot reviewed Sep 5, 2022

View reviewed changes

BUG: fix a regression where tick direction wasn't conserved by Axis.c…

92cab09

…lear()

neutrinoceros force-pushed the hotfix_23806 branch from 0f92ac5 to 92cab09 Compare September 6, 2022 09:25

neutrinoceros mentioned this pull request Sep 6, 2022

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

Closed

tacaswell approved these changes Sep 6, 2022

View reviewed changes

tacaswell added this to the v3.6.0 milestone Sep 6, 2022

jklymak approved these changes Sep 6, 2022

View reviewed changes

ENH: preserve more style parameters in Axis._reset_major_tick_kw (res…

e590c5e

…p minor)

timhoffm reviewed Sep 8, 2022

View reviewed changes

TST: simplify and comment test

48a5382

timhoffm mentioned this pull request Sep 8, 2022

Revert "Refactor handling of tick and ticklabel visibility in Axis.clear" #23834

Merged

timhoffm marked this pull request as draft September 8, 2022 21:39

timhoffm added the status: needs comment/discussion needs consensus on next step label Sep 8, 2022

jklymak mentioned this pull request Sep 9, 2022

[MNT]: Add tests to codify ax.clear #23839

Closed

timhoffm modified the milestones: v3.6.0, unassigned Sep 9, 2022

story645 modified the milestones: unassigned, needs sorting Oct 6, 2022

github-actions bot added the status: needs rebase label Oct 18, 2022

neutrinoceros closed this Oct 4, 2023

neutrinoceros deleted the hotfix_23806 branch October 4, 2023 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: fix a regression where tick direction wasn't conserved by Axis.clear() #23808

BUG: fix a regression where tick direction wasn't conserved by Axis.clear() #23808

neutrinoceros commented Sep 5, 2022 •

edited

Loading

github-actions bot left a comment

jklymak left a comment

timhoffm commented Sep 7, 2022

neutrinoceros commented Sep 7, 2022 •

edited

Loading

neutrinoceros commented Sep 7, 2022

timhoffm left a comment

timhoffm Sep 7, 2022

neutrinoceros Sep 8, 2022

neutrinoceros Sep 8, 2022

neutrinoceros Sep 8, 2022

timhoffm Sep 8, 2022

timhoffm Sep 8, 2022

neutrinoceros Sep 8, 2022

neutrinoceros Sep 8, 2022

neutrinoceros commented Sep 8, 2022

timhoffm commented Sep 8, 2022

neutrinoceros commented Sep 8, 2022

timhoffm commented Sep 8, 2022

neutrinoceros commented Sep 9, 2022

neutrinoceros commented Oct 18, 2022

tacaswell commented Dec 16, 2022

neutrinoceros commented Oct 4, 2023

BUG: fix a regression where tick direction wasn't conserved by Axis.clear() #23808

BUG: fix a regression where tick direction wasn't conserved by Axis.clear() #23808

Conversation

neutrinoceros commented Sep 5, 2022 • edited Loading

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

timhoffm commented Sep 7, 2022

neutrinoceros commented Sep 7, 2022 • edited Loading

neutrinoceros commented Sep 7, 2022

timhoffm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neutrinoceros commented Sep 8, 2022

timhoffm commented Sep 8, 2022

neutrinoceros commented Sep 8, 2022

timhoffm commented Sep 8, 2022

neutrinoceros commented Sep 9, 2022

neutrinoceros commented Oct 18, 2022

tacaswell commented Dec 16, 2022

neutrinoceros commented Oct 4, 2023

neutrinoceros commented Sep 5, 2022 •

edited

Loading

neutrinoceros commented Sep 7, 2022 •

edited

Loading