-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Enh better colorbar axes #18900
Conversation
lib/matplotlib/scale.py
Outdated
@@ -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() |
There was a problem hiding this comment.
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.
lib/matplotlib/testing/decorators.py
Outdated
@@ -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: |
There was a problem hiding this comment.
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.
ae2bdb3
to
3e6e303
Compare
a4e663c
to
d680c81
Compare
lib/matplotlib/colorbar.py
Outdated
self.__scale = 'log' | ||
if hasattr(self.norm, '_scale'): | ||
self.ax.set_xscale(self.norm._scale.name, | ||
**self.norm._scale._kwargs) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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.
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, its disruptive enough that it needs some buy in from everyone to break many of the colorbar tests... |
61a7088
to
92c733f
Compare
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) BeforeAfter |
On the call it was decided that clipping the contour lines was undesirable, so will add logic to fix that. |
ping @efiring @dopplershift about above issue. |
IMO, if 100 is potentially a colored contour in that image above, it's a mistake to clip it. |
@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. |
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. |
9eeb460
to
5abb157
Compare
dfbbcfc
to
fd2a813
Compare
7ba152d
to
09cad29
Compare
0203488
to
abd1aa6
Compare
There was a problem hiding this 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.
lib/matplotlib/colorbar.py
Outdated
# 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): |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
5198d87
to
6f88ba3
Compare
Ill start pinging for reviews on this. Thanks @greglucas for your input! |
6f88ba3
to
df33845
Compare
@@ -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) |
There was a problem hiding this comment.
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>
Somehow github won't let me re-open this PR or force push to it... |
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. |
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 proportionextendlen
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
andvmax
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:
Cons:
manual axes placement is not the main colorbar axes object any more:
The biggest con is that for
the axes object we create is new so that
cb.ax
is no longercax
. In factcax == cb.ax.parent_axes
. So this breaks cases where the user hoped to do things tocax
after it was created. Of course they can accesscb.ax
. This failed one test where we didcax.tick_params
. We can pass these through as we find them (I did already fortick_params
). However, this also means thatcax.xaxis
points to an axis we make invisible now, and won't do anything (againcb.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 oncb.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
, andcontour_manual_colors_and_levels.png
)Other changes:
double_cbar.png
and a few other tests.double_cbar.png
)_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:
Before
After
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).