Colorbar redo again! #20501

jklymak · 2021-06-23T23:25:45Z

PR Summary

OK, third time is the charm.

Previous overhaul packaged an inner and outer axes in a container "ColorbarAxes" and tried to dispatch methods between them.

This overhaul takes the much simpler approach of resizing the image using a custom _axes_locator that a) calls any existing locator b) or just uses the axes default position. The custom _axes_locator then shrinks the axes in the appropriate direction to make room for extend tri/rectangles. As with the previous fix, the extend tri/rectangles are drawn as patches in axes co-ordinates, rather than pcolormesh in "data" co-ordinates.

This addresses the concerns and closes #20479

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

jklymak · 2021-06-24T01:35:23Z

import numpy as np
import matplotlib.pyplot as plt
#from matplotlib.backend_bases import MouseButton

def pick_fn(mouseevent):
    if mouseevent.inaxes is colorbar.ax:
        print('pick\n\nPICKKKKKKK')
        print(mouseevent.ydata, mouseevent.button)


def motion_fn(mouseevent):
    if mouseevent.inaxes is colorbar.ax:
        print(mouseevent.ydata, mouseevent.button)
        pass


fig, ax = plt.subplots()
canvas = fig.canvas
arr = 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 to
colorbar.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()

works fine on this PR.

lib/matplotlib/axes/_base.py

jklymak · 2021-06-24T03:30:33Z

lib/matplotlib/axes/_base.py

@@ -2369,6 +2369,8 @@ def _update_patch_limits(self, patch):
            return
        patch_trf = patch.get_transform()
        updatex, updatey = patch_trf.contains_branch_seperately(self.transData)
+        if not (updatex or updatey):
+            return


Calling the transform below blows up needlessly because it is called, and then does nothing....

jklymak · 2021-06-24T16:22:08Z

... interesting wrinkle. If an axes has an _axes_locator that is not None, tight_layout decides it can't handle it. Here the locator only is supposed to modify things in the direction perpendicular to the one tight_layout needs to worry about, so presumably it should be fine, but the warning still persists and breaks the docs build.

greglucas · 2021-06-26T17:42:51Z

I have to say, this was actually fewer image changes than I was expecting :)
I have a bunch of small nitpicks in there, but overall I think this is a great simplification and really makes the logic easier to understand what is going on than the multi-axes dispatching.

lib/matplotlib/tests/test_colorbar.py

greglucas · 2021-06-26T16:15:40Z

lib/matplotlib/tests/test_colorbar.py

-    np.testing.assert_allclose(cb.ax.get_position().extents,
-                               [0.78375, 0.536364, 0.796147, 0.9], rtol=2e-3)
-
-


Surely this test was there for a reason, especially saying it was a "funny" error. Makes it seem like this should be investigated and updated rather than removed.

This was testing the extend length being correct (its my test). We could go ahead and keep testing this, but it'd properly require an image test.

lib/matplotlib/tests/test_colorbar.py

lib/matplotlib/tests/test_axes.py

greglucas · 2021-06-26T16:59:19Z

lib/matplotlib/colorbar.py

+        except TypeError:
+            pass
+        lim = self._cbar._long_axis().get_view_interval()
+        self._cbar._short_axis().set_view_interval(*lim)


Do you also want to update the data interval of the short axis here?

I'm not sure what you mean? We need to do this to make the aspect ratio of the colobar correct.

OK, this has been removed. See comments

lib/matplotlib/colorbar.py

greglucas · 2021-06-26T17:14:09Z

lib/matplotlib/colorbar.py

+        if self._cbar.orientation == 'vertical':
+            if aspect:
+                ax.set_aspect(aspect*shrink)
+            offset = offset * pos.height


*= or even just explicitly put offset * pos.height into the translated call below.

greglucas · 2021-06-26T17:28:07Z

lib/matplotlib/colorbar.py

-
-        # make the inner axes smaller to make room for the extend rectangle
-        top = elower + inner_length
+        top = eupper + 1


For consistency I'd suggest either defining bot = -elower here and using that below or using 1 + eupper in the calculations.

greglucas · 2021-06-26T17:39:13Z

lib/matplotlib/colorbar.py

-            self.ax.outer_ax.add_patch(patch)
+                antialiased=False, transform=self.ax.transAxes,
+                hatch=hatches[0], clip_on=False)
+            self.ax.add_patch(patch)


Because you're setting clip_on=False here, do you need to adjust the zorder of the patch at all so that it is behind the main axes?

I think this is as it was before?

This seems reasonable with the layer between the patch, the outline, and then the fill-ins for the extensions.

We do not keep a (named) reference to these patches, what do we do if we update the colormap?

QuLogic · 2021-06-30T03:44:25Z

lib/matplotlib/tests/test_colorbar.py

+            Colorbar(cax, cmap=cmap, norm=norm,
                         boundaries=boundaries, values=values,
                         extend=extension_type, extendfrac=extendfrac,
                         orientation='horizontal', spacing=spacing)


Dedent these also.

lib/matplotlib/tests/test_colorbar.py

lib/matplotlib/colorbar.py

QuLogic · 2021-06-30T03:57:10Z

lib/matplotlib/colorbar.py

+
+    def __call__(self, ax, renderer):
+
+        # make sure that lims and scales are the same


I'm not sure why this is necessary; I understand from some comments that this is due to aspect ratio handling?

Right, the aspect of the axes is presently set via the usual ax.set_aspect, but that requires the limits and scales of the short and long axes to be the same.

However I'm starting to think that is a mistake, and we should perhaps just use something like set_box_aspect and get around all this awkwardness. Or just do it with the new colorbar axes locator.

See comments, this has all be re-engineered.

jklymak · 2021-06-30T21:00:10Z

latest commit changes the aspect ratio handling from a data-aspect ratio to a box aspect ratio. The "short" side of the colorbar now always has a linear scale and limits from 0-1, and only the "long" side gets a scale and variable data limits. This simplifies a lot of code..

Note I've added a colorbar.set_aspect and colorbar.set/get_scale. ~~These will need whats-new entries~~ (done)

lib/matplotlib/colorbar.py

QuLogic · 2021-07-01T08:34:47Z

lib/matplotlib/colorbar.py

-    cax.set_aspect(aspect, anchor=loc_settings["anchor"], adjustable='box')
+    cax.set_anchor(anchor)


Was loc_settings["anchor"] an error?

Yeah, I assume that anchor was just ignored if passed as a kwarg. I'm not sure what happens if it is passed as a kwarg.

Strange, it should just go through to set_anchor if it's not None.

I think what is happening here is correct, and the old code was ignoring the user's specification of anchor, but maybe I'm not following...

lib/matplotlib/colorbar.py

greglucas · 2021-07-01T14:25:39Z

Are the set_scale and set_aspect really necessary public functions on Colorbar and not just on the internal axis?

set_aspect: This is generally on an Axes, so why not leave it on cbar.ax and let the new locator work on that?

set_scale: I feel like this could cause confusion. The norm of the colorbar can be out of sync with the scale now? If I call cbar.set_scale('log')`, the ticks would be located in log-space, but the color normalization still done in linear space I believe.

jklymak · 2021-07-01T15:32:08Z

Are the set_scale and set_aspect really necessary public functions on Colorbar and not just on the internal axis?

set_aspect: This is generally on an Axes, so why not leave it on cbar.ax and let the new locator work on that?

This is the same as colorbar(..., aspect=20), so it really is a colorbar method. The equivalent axes method is set_box_aspect, and indeed set_aspect will not work.

set_scale: I feel like this could cause confusion. The norm of the colorbar can be out of sync with the scale now? If I call cbar.set_scale('log')`, the ticks would be located in log-space, but the color normalization still done in linear space I believe.

I needed to make this method, and didn't see that it needed to be private, but I don't feel strongly about it. Indeed it will only change the scale and not have anything to do with the norm's scale.

greglucas · 2021-07-01T15:51:35Z

Your new AxesLocator is the only thing calling cbar.set_aspect though because you just put it in. I'm suggesting that you can put these two calls up in the locator instead, without introducing new public API. (You added these two calls into ConstrainedLayout already, so I think the same idea could be applied in the Locator)

        self.ax.set_box_aspect(aspect)
        self.ax.set_aspect('auto')

Indeed it will only change the scale and not have anything to do with the norm's scale.

This seems like a real reason not to make this public. In your calls to self.set_scale(), you already have the long axis attribute above, so why not call self._long_axis().set_scale() there instead of introducing the new API?

jklymak · 2021-07-01T16:05:00Z

I'm suggesting that you can put these two calls up in the locator instead, without introducing new public API.

Absolutely. I'm just suggesting it may be more-generally useful as well to be able to set this after creating the colorbar, so why not expose it?

so why not call self._long_axis().set_scale() there instead of introducing the new API?

Yes, there is no reason for it to be public, other than user convenience. As I said, I don't feel super strongly about exposing this, but we have had requests to change the scale without changing the norm. For instance, it is not completely unreasonable to have a linear scale and a logarithmic norm.

lib/matplotlib/colorbar.py

QuLogic · 2021-07-06T02:30:44Z

lib/matplotlib/colorbar.py

-    cax.set_aspect(aspect, anchor=loc_settings["anchor"], adjustable='box')
+    cax.set_anchor(anchor)


Strange, it should just go through to set_anchor if it's not None.

lib/matplotlib/tests/test_colorbar.py

lib/matplotlib/colorbar.py

QuLogic · 2021-07-07T08:41:49Z

I assume you'll fix the last thing.

jklymak · 2021-07-07T13:51:11Z

@QuLogic, sorry, what "last thing"? The anchor passthrough? I think the new code is correct...

greglucas

I think this is good to go as well and the other issues with the norms/scales can be addressed in later PRs. Thanks for the updates and separating things out @jklymak! I'll let @QuLogic merge once he feels his comment is resolved.

QuLogic · 2021-07-07T19:09:01Z

@QuLogic, sorry, what "last thing"? The anchor passthrough? I think the new code is correct...

#20501 (comment) longax wasn't removed.

tacaswell

Anyone can merge on CI green!

Previous overhaul packaged an inner and outer axes in a container "ColorbarAxes" and tried to dispatch methods between them. This overhaul takes the _much_ simpler approach of resizing the image using a custom _axes_locator that a) calls any existing locator b) or just uses the axes default position. The custom _axes_locator then shrinks the axes in the appropriate direction to make room for extend tri/rectangles. As with the previous fix, the extend tri/rectangles are drawn as patches in axes co-ordinates, rather than pcolormesh in "data" co-ordinates.

jklymak · 2021-07-08T00:12:39Z

Thanks @QuLogic and @greglucas for all your help!

jklymak added the topic: color/colorbar label Jun 23, 2021

jklymak mentioned this pull request Jun 23, 2021

FIX: ColorbarAxes properly in the axes stack #20484

Closed

7 tasks

jklymak force-pushed the colorbar-try3-justmod-transform branch from 4becc5b to 114e8d3 Compare June 24, 2021 00:39

jklymak commented Jun 24, 2021

View reviewed changes

lib/matplotlib/axes/_base.py Outdated Show resolved Hide resolved

jklymak force-pushed the colorbar-try3-justmod-transform branch 3 times, most recently from 0370b32 to 735de0d Compare June 24, 2021 03:28

jklymak commented Jun 24, 2021

View reviewed changes

jklymak force-pushed the colorbar-try3-justmod-transform branch 2 times, most recently from eab1f92 to 32b38d2 Compare June 24, 2021 05:17

jklymak added this to the v3.5.0 milestone Jun 24, 2021

jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jun 24, 2021

jklymak marked this pull request as ready for review June 24, 2021 05:20

jklymak force-pushed the colorbar-try3-justmod-transform branch from 32b38d2 to 561d771 Compare June 24, 2021 14:32

jklymak mentioned this pull request Jun 24, 2021

Deprecate Colorbar.patch. #20481

Merged

7 tasks

greglucas mentioned this pull request Jun 25, 2021

matplotlib 3.5 breaks yt #20520

Closed

greglucas linked an issue Jun 26, 2021 that may be closed by this pull request

matplotlib 3.5 breaks yt #20520

Closed

greglucas reviewed Jun 26, 2021

View reviewed changes

QuLogic reviewed Jun 30, 2021

View reviewed changes

jklymak force-pushed the colorbar-try3-justmod-transform branch from 182a488 to b573664 Compare June 30, 2021 20:56

QuLogic reviewed Jul 1, 2021

View reviewed changes

jklymak force-pushed the colorbar-try3-justmod-transform branch from 514192f to e391e62 Compare July 1, 2021 16:44

jklymak force-pushed the colorbar-try3-justmod-transform branch from 70621cd to 5a5d1bb Compare July 6, 2021 00:32

QuLogic reviewed Jul 6, 2021

View reviewed changes

jklymak force-pushed the colorbar-try3-justmod-transform branch from f0ca1e0 to 914b750 Compare July 6, 2021 13:42

QuLogic reviewed Jul 6, 2021

View reviewed changes

lib/matplotlib/colorbar.py Show resolved Hide resolved

tacaswell reviewed Jul 7, 2021

View reviewed changes

lib/matplotlib/colorbar.py Outdated Show resolved Hide resolved

greglucas reviewed Jul 7, 2021

View reviewed changes

lib/matplotlib/colorbar.py Show resolved Hide resolved

lib/matplotlib/colorbar.py Outdated Show resolved Hide resolved

jklymak mentioned this pull request Jul 7, 2021

ENH colorbar.set_aspect #20588

Closed

7 tasks

QuLogic approved these changes Jul 7, 2021

View reviewed changes

greglucas approved these changes Jul 7, 2021

View reviewed changes

jklymak force-pushed the colorbar-try3-justmod-transform branch 2 times, most recently from 8caf7dd to 7884314 Compare July 7, 2021 20:08

tacaswell approved these changes Jul 7, 2021

View reviewed changes

jklymak force-pushed the colorbar-try3-justmod-transform branch from 7884314 to b574177 Compare July 7, 2021 20:44

jklymak force-pushed the colorbar-try3-justmod-transform branch from b574177 to 277b5eb Compare July 7, 2021 20:46

jklymak merged commit 4d1cd5e into matplotlib:master Jul 8, 2021

jklymak deleted the colorbar-try3-justmod-transform branch July 8, 2021 00:12

Zeroto521 mentioned this pull request Jul 8, 2021

TST: adapt colorbar tests to changes in matplotlib master geopandas/geopandas#1962

Closed

jorisvandenbossche mentioned this pull request Jul 8, 2021

TST: adapt again colorbar tests to matplotlib 3.5 changes geopandas/geopandas#2001

Merged

richardsheridan mentioned this pull request Jul 8, 2021

add interactive colorbar example to gallery #20471

Merged

7 tasks

greglucas mentioned this pull request Jul 9, 2021

Colorbar axis zoom and pan #19515

Merged

7 tasks

jklymak mentioned this pull request Jul 28, 2021

Fix cla colorbar #20330

Closed

7 tasks

timhoffm mentioned this pull request Sep 2, 2021

[Bug]: broken 'proportional' colorbar when using contourf+cmap+norm+extend #20963

Closed

ericpre removed the status: needs review label Oct 17, 2021

This was referenced Jan 3, 2022

Fix colorbar aspect when colorbar created, not its axes... #22100

Closed

[Bug]: aspect ratio control of colorbar Axes fails. #22087

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colorbar redo again! #20501

Colorbar redo again! #20501

jklymak commented Jun 23, 2021 •

edited

Loading

jklymak commented Jun 24, 2021

jklymak Jun 24, 2021

jklymak commented Jun 24, 2021

greglucas commented Jun 26, 2021

greglucas Jun 26, 2021

jklymak Jun 30, 2021

greglucas Jun 26, 2021

jklymak Jun 30, 2021

jklymak Jun 30, 2021

greglucas Jun 26, 2021

greglucas Jun 26, 2021

greglucas Jun 26, 2021

jklymak Jun 30, 2021

tacaswell Jul 7, 2021

QuLogic Jun 30, 2021

QuLogic Jun 30, 2021

jklymak Jun 30, 2021

jklymak Jun 30, 2021

jklymak commented Jun 30, 2021 •

edited

Loading

QuLogic Jul 1, 2021

jklymak Jul 1, 2021

QuLogic Jul 6, 2021

jklymak Jul 6, 2021

greglucas commented Jul 1, 2021

jklymak commented Jul 1, 2021

greglucas commented Jul 1, 2021

jklymak commented Jul 1, 2021

QuLogic Jul 6, 2021

QuLogic commented Jul 7, 2021

jklymak commented Jul 7, 2021

greglucas left a comment

QuLogic commented Jul 7, 2021

tacaswell left a comment

jklymak commented Jul 8, 2021

		np.testing.assert_allclose(cb.ax.get_position().extents,
		[0.78375, 0.536364, 0.796147, 0.9], rtol=2e-3)


		def __call__(self, ax, renderer):

		# make sure that lims and scales are the same

		cax.set_aspect(aspect, anchor=loc_settings["anchor"], adjustable='box')
		cax.set_anchor(anchor)

Colorbar redo again! #20501

Colorbar redo again! #20501

Conversation

jklymak commented Jun 23, 2021 • edited Loading

PR Summary

PR Checklist

jklymak commented Jun 24, 2021

Choose a reason for hiding this comment

jklymak commented Jun 24, 2021

greglucas commented Jun 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglucas commented Jul 1, 2021

jklymak commented Jul 1, 2021

greglucas commented Jul 1, 2021

jklymak commented Jul 1, 2021

Choose a reason for hiding this comment

QuLogic commented Jul 7, 2021

jklymak commented Jul 7, 2021

greglucas left a comment

Choose a reason for hiding this comment

QuLogic commented Jul 7, 2021

tacaswell left a comment

Choose a reason for hiding this comment

jklymak commented Jul 8, 2021

jklymak commented Jun 23, 2021 •

edited

Loading

jklymak commented Jun 30, 2021 •

edited

Loading