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

Adding cla and remove to ColorbarAxes#20323

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

Closed

Conversation

greglucas
Copy link
Contributor

PR Summary

Noticed while testing re-using the same axes to place a new colorbar in. These methods need to be applied to both axes.

PR Checklist

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • [N/A] New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • [N/A] New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

The cla and remove methods need to be applied to both theinner and outer axes.
@jklymak
Copy link
Member

I don't think it's this easy. What happens if you pass different length extends to the second colorbar call? Cla is not adequate to change the colorbar.

Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'll block just to stop this getting merged too fast. The semantics of cla on a colorbar axes are not the same as a normal axes.

@jklymak
Copy link
Member

It is a little strange to clear a colorbar axes, and/or reuse it. Why are you doing this? I'm 90% surecb.ax.cla didn't do anything sensible before#20054 either.

Certainly as it stands, this PR is adding andinner_ax to aColorbarAxes that already has aninner_ax and that is an unacceptable mess. It will work, but will lead toself.outer.inner existing, and if you start changing the extend lengths this will lead to very funny axes:

cb=fig_test.colorbar(pc,extend='both')# Clear and re-use the same colorbar axescb.ax.cla()cb=fig_test.colorbar(pc,cax=cb.ax)

won't work.

I suppose thatcb.ax.cla should just undo the nesting and return a non colorbar axes. If that is what we want, probably something like:

def cla(self):   fig = self.outer_ax.figure   pos = self.outer_ax.get_position()   new = fig.add_axes(pos)   ax_loc = self.outer_ax._axes_locator   self.outer_ax.delete()   self.innerer_ax.delete()   self = new   self._axes_locator = ax_loc

but I'm not 100% sure that garbage collects properly.

@greglucas
Copy link
ContributorAuthor

I agree, I'm not entirely sure how useful/practical this is. It came up when I was looking into the tests for axes_grid where the cax is being re-used, and I was trying to see if the same thing happens for regular axes with the state sharing. I couldn't even test that case because there isn't a remove method on the CbarAxes, so it throws an error currently.
(See ax.cax.cla() in this test here)

# In each row/column, the "first" colorbars must be overwritten by the
# "second" ones. To achieve this, clear out the axes first.
foraxingrid:
ax.cax.cla()
cb=ax.cax.colorbar(ax.images[0])

@jklymak
Copy link
Member

I guess changing the mappable associated with the colorbar is a reasonable use-case.

If we think the semantics ofcla is "get back to the outer axes passed by the user stripped of any colorbar info", I think the above suggestion is the right track.

@tacaswell
Copy link
Member

I think it is reasonable to expect thatcax.cla() does the right thing at at least visually restores the Axes to the "blank" state.

This is pushing towards type promotion being the better option here as it lets us "demote" the Axes back to a normal axes incla.

colorbar probably also needs to pick up some logic to say "if the cax I've been given has already been promoted at least once, re-use the inner/outer logic rather than re-adding it". I now suspect (but have not chased through to be sure) that this is the core issue in#19553 's failing tests.

@jklymak
Copy link
Member

I guess the question is if#19553 was failing before#20054 was rebased into it. However, that test passed#20054, so I'm not clear why this PR causes problems.

This is pushing towards type promotion being the better option here as it lets us "demote" the Axes back to a normal axes in cla.

I agree that demoting is the right thing. I'm not quite sure what you mean by "type promotion", and I don't see anything in the conversations of#20054. If you mean casting the original axes toColorbarAxes that is basically what we are doing, so I'm not clear how it simplifies things. Certainly we would have to do some sort of dance like above...

@jklymak
Copy link
Member

defcla(self):"""        Reset the colorbar axes to be empty.        """# need to low-level manipulate the stacks because we# are just swapping places here.  We don't need to# set transforms etc...self.figure._axstack.add(self)self.figure._axstack.remove(self.outer_ax)self.figure._localaxes.add(self)self.figure._localaxes.remove(self.outer_ax)self.__dict__.update(self.outer_ax.__dict__)self.inner_ax.remove()delself.inner_axdelself.outer_axself.xaxis.set_visible(True)self.yaxis.set_visible(True)forspineinself.spines.values():spine.set_visible(True)self.set_facecolor('white')

seems to work fine, except the callback for the mappable gets called while the axes is in the indeterminate state for some reason, so we need a try/except ondraw_all (did I mention I don't like callbacks?)

Setting all the axis visibility/facecolor is a bit hokey, as we should probably set the state to what it was before colorbar was called? But I'm not sure anyone is going to be using the axes for anything but a new colorbar, so maybe that is OK?

@jklymakjklymak mentioned this pull requestMay 28, 2021
7 tasks
@greglucas
Copy link
ContributorAuthor

Closing in favor of#20330

@greglucasgreglucas deleted the cbar-cla-remove branchMay 28, 2021 23:57
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jklymakjklymakjklymak requested changes

@tacaswelltacaswelltacaswell approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

4 participants
@greglucas@jklymak@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp