Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pizzathief
Copy link
Contributor

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:
Figure_1

@timhoffm
Copy link
Member

timhoffm commented Jun 2, 2024

Quoting #28268 (comment)

The core problem is that there is some ambiguity as to what "Clearing" as axes means and it is double ambiguous with shared axis.

While I haven't dug into the details of what we currently do, these may be reasonble guildelines

  • All data-like content should be removed (actual data, labels, etc.)
  • Layout should stay (position, twinning)
  • Formatting: t.b.d.

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.

@timhoffm timhoffm added the status: needs comment/discussion needs consensus on next step label Jun 2, 2024
@jklymak
Copy link
Member

jklymak commented Jun 2, 2024

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)

@timhoffm
Copy link
Member

timhoffm commented Jun 2, 2024

Another option is that twinned axes become their own class that is particularly a child of the parent axes.

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.

@jklymak
Copy link
Member

jklymak commented Jun 2, 2024

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 clear is called.

I do agree, however, that the concept of clear should be specified, rather than left up to interpretation of successive contributors.

@tacaswell
Copy link
Member

As a side note, welcome back! After #5405 I feared you had burned out on mpl work.

Copy link
Member

@timhoffm timhoffm left a 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

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.

@timhoffm
Copy link
Member

timhoffm commented Sep 30, 2024

While we still have to clarify the exact semantics of clear (#28851), I agree that twinning should be kept throughout clear, and keeping the moved label is part of that. We could accept this PR as a stopgap fix. However, I'm unclear whether this fixes all aspects of twinx. Compare

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.

@pizzathief
Copy link
Contributor Author

With this patch:

❯ git diff _base.py
diff --git a/lib/matplotlib/axes/_base.py b/lib/matplotlib/axes/_base.py
index 433647516f..2fc6c22901 100644
--- a/lib/matplotlib/axes/_base.py
+++ b/lib/matplotlib/axes/_base.py
@@ -1269,6 +1269,13 @@ class _AxesBase(martist.Artist):
             set_right_label_again = \
                 (self._axis_map['y'].get_label_position() == 'right')
 
+        # A twiny-copied axis title set to the top will move back the
+        # bottom if it is cleared.
+        set_top_label_again = False
+        if 'x' in self._axis_map:
+            set_top_label_again = \
+                (self._axis_map['x'].get_label_position() == 'top')
+
         if hasattr(self, 'patch'):
             patch_visible = self.patch.get_visible()
         else:
@@ -1388,6 +1395,8 @@ class _AxesBase(martist.Artist):
 
         if set_right_label_again:
             self._axis_map['y'].set_label_position('right')
+        if set_top_label_again:
+            self._axis_map['x'].set_label_position('top')
 
         self.stale = True

The following adjusted test code

import matplotlib.pyplot as plt

figure = plt.figure()
ax = figure.add_subplot(111)
ax2 = ax.twinx()
ax2.cla()#or clear() ; doesn't work both ways
ax.set_ylabel('ax_label')
ax.set_xlabel('ay_label')
ax2.set_ylabel('ax2_label')#expected to be on the right

ax3 = ax.twiny()
ax3.cla()
ax3.set_xlabel('ax3_label')

plt.show()

has before and after images:
Before patch
Figure_1
After Patch
Figure_2

Did you want it to restore the other properties as well (eg autoscale, visible, units) or is this sufficient?

@timhoffm
Copy link
Member

timhoffm commented Oct 2, 2024

looking at what was set during twinx(), I believe we want all these settings to persist after clear

 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 
  • Ticks, label and offset label should be right
  • Autoscaling should be on - we're erased all data and the limits should be autoscaled when adding new data
  • xaxis and patch should be invisible.
  • xaxis units should match (it may be that they always do after clearing but t.b.d.)

I haven't checked which of these properties are reset through clear. But if they are reset, you should restore them.

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

Successfully merging this pull request may close these issues.

[Bug]: y_label in wrong place after clearing for twinx axes
4 participants