Skip to content

Enh better colorbar axes #18900

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

Closed
wants to merge 3 commits into from

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Nov 5, 2020

PR Summary

This is a relatively major re-arrangement of how we create colorbars. This will simplify making colorbars for other norms if the norms are created with a scale. It also allows more normal manipulation of the colorbar axes. There are some negative consequences as well.

Issue

Currently most colorbars are drawn as a pcolormesh on a hidden axes. When extend='both'/'upper'/'lower' it makes the axes longer by a proportion extendlen and draws the extend regions by distorting the upper and/or lower cell of the pcolormesh. This is great, except it means that all the usual tick locators need to have special cases that do not continue drawing the ticks into the region of the axis that has the extend triangles. We do that now with a few piecemeal wrapper classes: _ColorbarAutoLocator(ticker.MaxNLocator), _ColorbarAutoMinorLocator(ticker.AutoMinorLocator), etc. Needless to say that is clunky.

The scale used for the colorbar also has to nicely invert past vmin and vmax for the triangles to be drawn properly, despite the fact these regions are only meant for over/under colors that you wouldn't necessarily expect an arbitrary norm to be able to handle.

Proposal

The proposed solution here is to draw the main pcolor ("solid") on a full axes that goes from vmin to vmax, and draw the extend triangles ("extends") as patches appended to the end of that axes. They are drawn in axes space, so the scale of the axes and the limits of the axes no longer matter.

The problem with this approach is that all the sizing and placement of axes is with an axes that is the size of the solid and the extends. i.e. if shrink=1 the whole axes, including the extends, is the height (or width) of the parent axes.

The solution proposed here is to draw the solid on an inset_axes that is shrunk from the parent axes if there are extends.

Pros:

  • The visible axis labels are all on the "inner" axes and no longer require weird wrappers around the tick Locators.
  • Any scale can be used, and the user can set the scale manually as they like. The initial scale is chosen from the norm, if it has one, or chosen to be linear.
  • the user can change the min and max of the axes if they like. We sometimes get requests for this if the user wants to zoom in on some part of the colormap.
  • I think the code is simpler, but I wrote it so YMMV

Cons:

manual axes placement is not the main colorbar axes object any more:

The biggest con is that for

cax = fig.add_axes([0.1, 0.1, 0.8, 0.07])
cb = fig.colorbar(im, cax=cax)

the axes object we create is new so that cb.ax is no longer cax. In fact cax == cb.ax.parent_axes. So this breaks cases where the user hoped to do things to cax after it was created. Of course they can access cb.ax. This failed one test where we did cax.tick_params. We can pass these through as we find them (I did already for tick_params). However, this also means that cax.xaxis points to an axis we make invisible now, and won't do anything (again cb.ax.xaxis points to the right thing.)

I will argue this breakage is worth it. Doing random things to cax failed much of the time anyway, whereas the new object stored on cb.ax should be much more robust. Unfortunately, however, I cannot think of a reasonable way to deprecate this.

Slight change in visibility of lines with extends

This is an obscure one, but lines are now partly cut off if they are drawn on the edge of the colorbar and there is an extend. The test that broke this didn't appear to have a reasonable reason to have an extend in that case anyhow (see contour_colorbar.png, and contour_manual_colors_and_levels.png)

Other changes:

  1. There seemed to have been a bug in colorbar widths with extends before, where they ended up a bit narrower than the colorbars without extends. This can be seen in double_cbar.png and a few other tests.
  2. If there is only one extend, the new axes is offset from the center. This means that the label (which is attached to the inner_axes) is now offset as well. We could move it (by placing the label logic all on the parent), but I'm not convinced that it doesn't look better to let it move down and be centred on the spine. (Also double_cbar.png)
  3. I ditched the private _internal.classic_mode for the colorbars. It doesn't change that many tests, and it needlessly complicates the Locator code to keep it around.

Test:

fig, ax = plt.subplots(1, 2, constrained_layout=True)
pc = ax[0].pcolormesh(np.arange(100).reshape(10, 10)+1)
cb = fig.colorbar(pc, ax=ax[0], extend='both')
cb.ax.set_yscale('log')

pc = ax[1].pcolormesh(np.arange(100).reshape(10, 10)+1)
cb = fig.colorbar(pc, ax=ax[1], extend='both')
cb.ax.set_ylim([20, 90])
plt.show()

Before

before

After

after

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).

@@ -53,6 +53,7 @@ def __init__(self, axis):
be used: a single scale object should be usable by multiple
`~matplotlib.axis.Axis`\es at the same time.
"""
self._kwargs = dict()
Copy link
Member Author

Choose a reason for hiding this comment

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

Norms now make themselves out of a scale. When the scale is created, some parameters are passed, and we need those if we are to re-recreate the scale for a colorbar.

@@ -122,7 +122,11 @@ def remove_ticks_and_titles(figure):
ax.zaxis.set_minor_formatter(null_formatter)
except AttributeError:
pass
for child in ax.child_axes:
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to do recursion on children.

@jklymak jklymak force-pushed the enh-better-colorbar-axes branch 2 times, most recently from ae2bdb3 to 3e6e303 Compare November 8, 2020 16:13
@jklymak jklymak force-pushed the enh-better-colorbar-axes branch 3 times, most recently from a4e663c to d680c81 Compare November 8, 2020 22:49
@jklymak
Copy link
Member Author

jklymak commented Nov 8, 2020

@efiring @anntzer you guys are probably most familiar w/ the colorbar code...

self.__scale = 'log'
if hasattr(self.norm, '_scale'):
self.ax.set_xscale(self.norm._scale.name,
**self.norm._scale._kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of storing kwargs to create another scale instance, perhaps you could instead modify Axes.set_scale (or add a new private Axes._set_scale_instance) that directly takes the scale instance as parameter instead? The scale docs explicitly state that a single scale instance should be usable on multiple axes at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wasn't brave enough to try that yet - we don't have, so far as I can tell, a public way to change the scale by just passing a scale instance to either the axes or axis. Perhaps that needs to be added, though I could do it using private things here....

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, that was easier than I thought. I just changed axis._set_scale to allow ScaleBase subclasses as well as strings. Publicly that now means that ax.set_xscale will allow a ScaleBase as an argument. I don't know why it didn't before.

@anntzer
Copy link
Contributor

anntzer commented Nov 9, 2020

Drawing on an inset_axes is a super nice trick; I've been wanting to move stuff to axes space for the longest time but never figured out how to handle the extenders...
No guarantees for a timely review, though.

@jklymak
Copy link
Member Author

jklymak commented Nov 9, 2020

No guarantees for a timely review, though.

No, its disruptive enough that it needs some buy in from everyone to break many of the colorbar tests...

@jklymak jklymak force-pushed the enh-better-colorbar-axes branch 2 times, most recently from 61a7088 to 92c733f Compare November 9, 2020 17:02
@jklymak
Copy link
Member Author

jklymak commented Nov 10, 2020

Just a demo of the aspect ratio behaviour. Note I could easily get the old behaviour if we want it back...

import numpy as np
import matplotlib.pyplot as plt
fig, ax = plt.subplots(2, 1, constrained_layout=False, figsize=(3, 3))

pc = ax[0].pcolormesh(np.arange(100).reshape(10, 10))
fig.colorbar(pc, ax=ax[0], aspect=5, shrink=1)

pc = ax[1].pcolormesh(np.arange(100).reshape(10, 10))
fig.colorbar(pc, ax=ax[1], extend='both', aspect=5, shrink=1, extendfrac=0.3)

Before

Before

After

After

@jklymak
Copy link
Member Author

jklymak commented Nov 10, 2020

On the call it was decided that clipping the contour lines was undesirable, so will add logic to fix that.

@jklymak
Copy link
Member Author

jklymak commented Nov 10, 2020

Question is: do we want the current behaviour for both extend and w/o extend? Because then we need a different code path for each.

Current behavior:

Neither

Old

New behaviour

Neither

Extend

@jklymak
Copy link
Member Author

jklymak commented Nov 10, 2020

ping @efiring @dopplershift about above issue.

@dopplershift
Copy link
Contributor

IMO, if 100 is potentially a colored contour in that image above, it's a mistake to clip it.

@jklymak
Copy link
Member Author

jklymak commented Nov 12, 2020

@dopplershift fair enough, but in both the case with and without the extend? Because it's a one-line fix if it can be same for both, considerably more work if only when the extends are present.

@dopplershift
Copy link
Contributor

I think it's good to plot the 100 contour in both cases (otherwise 100 is the only ticked value that doesn't have a colored line), so in this case, I think your simple fix should be good.

@greglucas greglucas mentioned this pull request Feb 15, 2021
7 tasks
@jklymak jklymak linked an issue Feb 19, 2021 that may be closed by this pull request
@jklymak jklymak force-pushed the enh-better-colorbar-axes branch 2 times, most recently from 7ba152d to 09cad29 Compare February 20, 2021 05:40
@jklymak jklymak added the status: needs comment/discussion needs consensus on next step label Feb 20, 2021
@jklymak jklymak force-pushed the enh-better-colorbar-axes branch from 0203488 to abd1aa6 Compare February 20, 2021 18:09
@jklymak jklymak marked this pull request as ready for review February 20, 2021 18:09
@jklymak jklymak removed the status: needs comment/discussion needs consensus on next step label Feb 20, 2021
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Wow, this is a hefty update! A really nice overhaul though I think.

# map some features to the parent so users have access...
self.parent_ax.tick_params = self.inner_ax.tick_params

def get_position(self, original=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like this to avoid the duplication of dispatching methods?

for attr in ["get_position", "set_position", "set_aspect"]:
    setattr(self, attr, getattr(self.parent_ax, attr))

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats a nice trick!

long_axis.set_minor_locator(_ColorbarAutoMinorLocator(self))
self.ax.minorticks_on()
self.minorlocator = self._long_axis().get_minor_locator()
self._short_axis().set_minor_locator(ticker.NullLocator())
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to set the minor locator? In draw_all() you set_ticks([], minor=True), which I think will remove the ticks already?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess? OTOH it doesn't hurt to be explicit?

@jklymak jklymak force-pushed the enh-better-colorbar-axes branch from 5198d87 to 6f88ba3 Compare February 23, 2021 05:42
@jklymak
Copy link
Member Author

jklymak commented Mar 7, 2021

Ill start pinging for reviews on this. Thanks @greglucas for your input!

@@ -451,13 +451,15 @@ def _get_pos_and_bbox(ax, renderer):
pos = pos.transformed(fig.transSubfigure - fig.transFigure)
try:
tightbbox = ax.get_tightbbox(renderer=renderer, for_layout_only=True)
print('tight', tightbbox)
Copy link
Member

Choose a reason for hiding this comment

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

These print statements should be removed?

Co-authored-by: David Stansby <dstansby@gmail.com>
@jklymak jklymak closed this Apr 23, 2021
@jklymak
Copy link
Member Author

jklymak commented Apr 23, 2021

Somehow github won't let me re-open this PR or force push to it...

@jklymak jklymak mentioned this pull request Apr 23, 2021
7 tasks
@jklymak
Copy link
Member Author

jklymak commented Apr 23, 2021

Sorry, this wasn't updating, so had to move to #20054. Thanks for reviews so far, I believe I addressed all the issues raised so far.

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

Successfully merging this pull request may close these issues.

Extra ticks at an extended part of colorbar
6 participants