Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
pizzathief wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
frompizzathief:bug_#28268

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 commentedJun 2, 2024
edited
Loading

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.

@timhoffmtimhoffm added the status: needs comment/discussionneeds consensus on next step labelJun 2, 2024
@jklymak
Copy link
Member

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

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

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

I do agree, however, that the concept ofclear 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

@timhoffmtimhoffm 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
returnax2

for what a twin Axes makes out.

Also, this should covertwiny as well.

@timhoffm
Copy link
Member

timhoffm commentedSep 30, 2024
edited
Loading

While we still have to clarify the exact semantics ofclear (#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
returnax2

for what a twin Axes makes out.

Also, this should covertwiny as well.

@pizzathief
Copy link
ContributorAuthor

With this patch:

❯ git diff _base.pydiff --git a/lib/matplotlib/axes/_base.py b/lib/matplotlib/axes/_base.pyindex 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 pltfigure = plt.figure()ax = figure.add_subplot(111)ax2 = ax.twinx()ax2.cla()#or clear() ; doesn't work both waysax.set_ylabel('ax_label')ax.set_xlabel('ay_label')ax2.set_ylabel('ax2_label')#expected to be on the rightax3 = 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

looking at what was set duringtwinx(), 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmtimhoffm left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: y_label in wrong place after clearing for twinx axes
4 participants
@pizzathief@timhoffm@jklymak@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp