Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
BUG: fix a regression where tick direction wasn't conserved by Axis.clear()#23808
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
160219b to0f92ac5CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
0f92ac5 to92cab09CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will hold off merging if@timhoffm wants to argue this is wrong, unless we are pushing 3.6 final out the door imminently...
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 |
neutrinoceros commentedSep 7, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 |
@timhoffm done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 regressionsax.clear() adds extra ticks, un-hides shared-axis tick labels #20721 andRegression: undocumented change of behaviour in mpl 3.4.2 with axis ticks direction #20219.
- these were hot-fixed inFix 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.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/tests/test_axes.py Outdated
| ax = fig_ref.subplots(1, 1) | ||
| ax.tick_params(**params) | ||
| ax.imshow(img) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there an advantage inimshow()? 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually I don't think I understand whatzorder=-1 issupposed to achieve, assuming the behaviour in matplotlib 3.5.3 isn't buggy. I would expect that combiningzorder=-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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
👍
| @@ -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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thislooks 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I intentionally left out grid params because I have no opinion there, but I agree it should be discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
zorder seemed to fit in the "visibility" theme, for which a consensus appears to have been reached. Please correct me if I am wrong.
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 |
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. |
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 @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. |
Thank you for your work, I really appreciate it ! |
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. |
This is unfortunately still in limbo. |
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. |
Uh oh!
There was an error while loading.Please reload this page.
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
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).