-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Restore axis title set to the right on a twinx() copied axis when cleared. #28325
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
base: main
Are you sure you want to change the base?
Conversation
Quoting #28268 (comment)
While I haven't dug into the details of what we currently do, these may be reasonble guildelines
Specifically this PR: a twinned Axes can only reasonable be (re-) used if the secondary label position is kept. I'm unsure whether the label position should be kept in general or only for twinned Axes. - I've a slight preference to keep in general, but that's really up for discussion. If we cannot decide now, the specific twinned solution is the minimal meaningful change - it should not break any user expectations, because keeping twinning, but resetting the label is inconsistent. The current implementation is too lax. Either one goes for the general solution, and keeps all label positions on all axes. Or one specifically keeps x-label positions for twinx and y-label position for twiny. And whatever the final decision/implementation is, we need tests to codify this behavior. |
Another option is that twinned axes become their own class that is particularly a child of the parent axes. This could make the class less flexible and allow us to enforce the fact it's a twin more robustly. In particular there seem to be perennial problems with resizing axes and draw order that could be more straightforward if it was clear an axes was a child of its parent (the name "twin" notwithstanding) |
I like the idea, but it‘s somewhat orthogonal to the problem here. We need to decide on the behavior of clear anyway, and implementation-wise a subclass is much more complicated than either of the concrete solutions here. The benefit would be on other aspects of twinning. |
I don't think it is very orthogonal - if twin axes are a subclass, we can define the default location of the twinned label versus setting it in an ad-hoc manner when the axes is created, and then allowing it to be changed to the global default when I do agree, however, that the concept of |
As a side note, welcome back! After #5405 I feared you had burned out on mpl work. |
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 agree that twinning should be kept throughout clear, and keeping the moved label is part of that. We could accept this as a stopgap fix. However, I'm unclear whether this fixes all aspects of twinx. Compare
matplotlib/lib/matplotlib/axes/_base.py
Lines 4575 to 4584 in 6404c95
ax2 = self._make_twin_axes(sharex=self) | |
ax2.yaxis.tick_right() | |
ax2.yaxis.set_label_position('right') | |
ax2.yaxis.set_offset_position('right') | |
ax2.set_autoscalex_on(self.get_autoscalex_on()) | |
self.yaxis.tick_left() | |
ax2.xaxis.set_visible(False) | |
ax2.patch.set_visible(False) | |
ax2.xaxis.units = self.xaxis.units | |
return ax2 |
for what a twin Axes makes out.
Also, this should cover twiny
as well.
|
looking at what was set during
I haven't checked which of these properties are reset through clear. But if they are reset, you should restore them. |
PR summary
When an axis is copied using twinx() the twin title is set to the right. If that axis is then cleared and its title text is set again,
the title is set back to the left. This PR alters that behaviour to be forced back to the right again.
closes #28268.
There was one test warning:
test test_pickle_load_from_subprocess
"This figure was saved with matplotlib version 1.6.0.dev34660+gae23583050.d19700101 and loaded with 1.6.0.dev34660+gae23583050.d20240530 so may not function correctly."
I'm hoping this doesn't appear on matplotlib's test runner.
Result of changes against bug example code:
