NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

FIX: ColorbarAxes properly in the axes stack#20484

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

Jump to bottom

Closed

jklymak wants to merge4 commits intomatplotlib:masterfromjklymak:fix-colorbar-axes-stack

Closed

FIX: ColorbarAxes properly in the axes stack#20484

jklymak wants to merge4 commits intomatplotlib:masterfromjklymak:fix-colorbar-axes-stack

Conversation

Copy link

Member

jklymak commentedJun 22, 2021•
edited
Loading

PR Summary

Closes#20479

ColorbarAxes was not sitting in the figure Axes stacks correctly - rather cb.outer_axes was still in the queue. Which worked fine, but didn't make mouseevents pass through properly.

The solution here is to swap cb.ax for cb.outer_axes in the figure axes stacks, and to redefinedraw on the ColorbarAxes to draw the outer axes first (i.e. set the aspect ratio and size of the bounding box for the colorbar).

I was testing on the following, though the real test is simply that cb.ax appears in the figure axes list now:

importnumpyasnpimportmatplotlib.pyplotasplt#from matplotlib.backend_bases import MouseButtondefpick_fn(mouseevent):ifmouseevent.inaxesiscolorbar.ax:print('pick\n\nPICKKKKKKK')print(mouseevent.ydata,mouseevent.button)defmotion_fn(mouseevent):ifmouseevent.inaxesiscolorbar.ax:print(mouseevent.ydata,mouseevent.button)passfig,ax=plt.subplots()canvas=fig.canvasarr=np.random.randn(100,100)axesimage=plt.imshow(arr)colorbar=plt.colorbar(axesimage,ax=ax,use_gridspec=True)# helps you see what value you are about to set the range tocolorbar.ax.set_navigate(True)canvas.mpl_connect("motion_notify_event",motion_fn)colorbar.ax.set_picker(True)canvas.mpl_connect("button_press_event",pick_fn)plt.show()

PR Checklist

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (runflake8 on changed files to check).
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).
New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

jklymak added topic: color/colorbar status: needs tests labels

Jun 22, 2021

jklymak force-pushed thefix-colorbar-axes-stack branch fromcda330a toaaaee32Compare

June 22, 2021 14:35

jklymak marked this pull request as draft

June 22, 2021 14:35

jklymak added2 commits

June 22, 2021 08:43

FIX: colorbarAxes properly in the axes stack

1b7bdfa

Make subplot_adjust work and remove work

eb879a9

jklymak force-pushed thefix-colorbar-axes-stack branch frome94d77c toeb879a9Compare

June 22, 2021 16:19

Flake8

f084fcb

jklymak force-pushed thefix-colorbar-axes-stack branch fromc2cf781 to4c7dbd7Compare

June 22, 2021 21:16

Deal with axes_grid differences

fb768c7

jklymak force-pushed thefix-colorbar-axes-stack branch from4c7dbd7 tofb768c7Compare

June 22, 2021 21:23

Copy link

MemberAuthor

jklymak commentedJun 22, 2021•
edited
Loading

... of course this turned out more complicated for some of the reasons I think@greglucas found - in particularaxes_grid had a pretty strange atypical way of adding colorbar axes. Basically the axes called a colorbar method on itself, which is exactly the opposite way the main library works, where the colorbar takes an axes and draws itself there. So the extra change here is to add the colorbar the usual way in that method. I may have been too cavalier in the ripping out untested axes_grid stuff, but at some point I despair of keeping incompatible convenience additions around unless they have tests...

Copy link

MemberAuthor

jklymak commentedJun 22, 2021

... 98.5% sure CI failures are unrelated to this PR (see#20487)

greglucas reviewed

Jun 22, 2021

View reviewed changes

Copy link

Contributor

greglucas left a comment

Choose a reason for hiding this comment

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

Bummer you ran into that here too! I didn't realize the AxesStack wasn't being updated, but this actually simplifies a lot of the interactive code I was working on as well, so thanks for this!

I never did get up to speed on theaxes_grid implementation, but making it more similar to a normal colorbar seems good to me. I guess I'd suggest that piece being a separate PR (or at least a separate commit) solely focused on that piece.

lib/matplotlib/colorbar.py

		else:
		outer_ax = parent

		# swap axes in the stack if its in there:

Copy link

Contributor

greglucasJun 22, 2021

Choose a reason for hiding this comment

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

I'm not sure how it wouldn't be there if you've created it above or gotten it passed in? But, wouldn't you want toadd(self) regardless of whether the outer one is there or not?

Copy link

MemberAuthor

jklymakJun 22, 2021

Choose a reason for hiding this comment

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

There was some test where we made a colorbar and it didn't actually ever get added. I can take ti out again to figure out which one...

lib/matplotlib/colorbar.py

		for attr in ["get_position", "set_position", "set_aspect"]:
		for attr in ["get_position", "set_aspect",
		"_remove_method", "_set_position",
		"set_position", "cla", "draw"]:

Copy link

Contributor

greglucasJun 22, 2021

Choose a reason for hiding this comment

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

I'm still not sure about these only being mapped to the outer_ax... I would have thought it should be something like:

defdraw(self,renderer):super().draw(renderer)self.outer_ax.draw(renderer)

so that you drawboth the inner and outer? Same for the cla and remove. The position's I think are appropriate to map straight to the outer though.

Copy link

MemberAuthor

jklymakJun 22, 2021

Choose a reason for hiding this comment

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

inner is an artist on outer, so inner gets drawn if outer gets drawn ;-)

Copy link

Contributor

greglucasJun 23, 2021

Choose a reason for hiding this comment

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

Oh yeah, it is an inset of the outer! Makes sense now, thanks for clarifying.

lib/matplotlib/colorbar.py

		if userax:
		self._colorbar_info = 'user'
		# point the parent's methods all at this axes...
		origdict = parent.__dict__
		parent.__dict__ = self.__dict__

Copy link

Contributor

greglucasJun 22, 2021

Choose a reason for hiding this comment

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

can you just update it?

Suggested change

	parent.__dict__=self.__dict__
	parent.__dict__.update(self.__dict__)

Copy link

MemberAuthor

jklymakJun 22, 2021

Choose a reason for hiding this comment

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

Oh, maybe? Thanks!

Copy link

MemberAuthor

jklymakJun 22, 2021

Choose a reason for hiding this comment

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

No, sadly not. I'll admit I'm probably not enough of a pythionista to dig around in the guts like this. Its pretty apparent to me that this copies all the attributes over, but if you iterate through__dict__ like a normal dictionary it does not capture everything in__dict__. Its all a little confusing. If someone has a more elegant less tacky way to do this, we can certainly try that approach...

Copy link

Contributor

greglucasJun 23, 2021

Choose a reason for hiding this comment

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

Maybe try{**parent.__dict__, **self.__dict__} which would produce a new dict...

Copy link

MemberAuthor

jklymak commentedJun 22, 2021

@greglucas thanks for taking a look. The problem with making the axes grid stuff a different PR is that of course the changes needed to get picking to work break axes_grid. (I honestly didn't set out to fix axes grid ;-)

Copy link

MemberAuthor

jklymak commentedJun 22, 2021

I guess the other approach here is rather than create a new container axes to just attach the stuff we want to the existing axes, and re-direct the appropriate attributes to the inner axes, keeping the outer axes as similar as possible. I avoided that route because I think far more of the "inner" methods are user-facing, but it certainly is difficult to do this dance robustly.

Copy link

MemberAuthor

jklymak commentedJun 23, 2021

OK, having thought about this a lot of the day, here is another approach:

Old, we did a bunch of drawing by hand
master: we replace the users cax incolorbar(im, cax=cax) by a newColorbarAxes and have acb.ax.inner_ax andcb.ax.outer_ax. This has the disadvantage ofassert cb.ax == cax failing, and is somewhat frail, as we feared.

New proposal: instead of trying to replacecax passed to us, we simply keep it, so nowassert cb.ax == cax is true. The colorbar gains a new attributecb.inner_ax that does the actual drawing etc except for the extend regions, and to make things easier on the user we dispatch many user-facing methods from cb.ax.set_xlabel to cb.inner_ax.set_xlabel. This is basically the same as the old changes but architected in a way that makes the user-supplied axes behave more like it used to (ideally exactly like it used to). The only disadvantage is thatinternally we need to be careful to callcolorbar.inner_ax.add_patch etc instead ofcolorbar.ax.add_patch. But since thats internal, it should be easy enough to handle.

QuLogic mentioned this pull request

Jun 23, 2021

Deprecate Colorbar.patch.#20481

Merged

7 tasks

Copy link

MemberAuthor

jklymak commentedJun 23, 2021

Closing for#20501

jklymak closed this

Jun 23, 2021

Labels

status: needs tests topic: color/colorbar

2 participants

Movatterモバイル変換

Uh oh!

FIX: ColorbarAxes properly in the axes stack#20484

FIX: ColorbarAxes properly in the axes stack#20484

Uh oh!

Conversation

jklymak commentedJun 22, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

jklymak commentedJun 22, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

jklymak commentedJun 22, 2021

Uh oh!

greglucas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commentedJun 22, 2021

Uh oh!

jklymak commentedJun 22, 2021

Uh oh!

jklymak commentedJun 23, 2021

Uh oh!

jklymak commentedJun 23, 2021

Uh oh!

Uh oh!

jklymak commentedJun 22, 2021•
edited
Loading

jklymak commentedJun 22, 2021•
edited
Loading