-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: adjustable colorbar ticks #9903
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
Conversation
OK, this failed enough tests that there are a bunch more edge cases I need to consider. Good thing there are tests. I'll probably re-do this from scratch and try to make it less invasive. I do think this is reasonable for 2.2, though, because I'll keep plugging away at it. |
9d1146d
to
5c48c10
Compare
This is redone, and much simpler now. 4 image files had to be changed... |
5c48c10
to
1d4160c
Compare
338c0a1
to
a31cadd
Compare
Ping @efiring The diff looks worse than it is because I moved a bit of code around. |
lib/matplotlib/colorbar.py
Outdated
|
||
self._set_label() | ||
|
||
def _get_ticker_locator_formatter(self): |
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.
Much of this was jut factored out of _ticker()
But note the new locators for LogNorm and the fallback...
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.
Some questions/comments, looks like this is good overall though
lib/matplotlib/colorbar.py
Outdated
This locator is just a `.MaxNLocator` except the min and max are | ||
clipped by the norm's min and max (i.e. vmin/vmax from the | ||
image/pcolor/contour object). This is necessary so ticks don't | ||
extrude into the "extend regions". |
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.
Shouldn't the docstring line up with the left of the quote marks?
lib/matplotlib/colorbar.py
Outdated
extrude into the "extend regions". | ||
""" | ||
|
||
def __init__(self, colorbar, *args, **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.
Why are there args and kwargs here if they're not used? Can this also get a docstring (just for the colorbar parameter is probably fine)
extrude into the "extend regions". | ||
|
||
""" | ||
def __init__(self, colorbar, *args, **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.
Same as above comment on __init__
lib/matplotlib/colorbar.py
Outdated
X, Y = self._mesh() | ||
C = self._values[:, np.newaxis] | ||
self._config_axes(X, Y) | ||
if self.filled: | ||
self._add_solids(X, Y, C) | ||
# self._set_view_limits() |
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.
Is this old debug code that needs to be removed?
lib/matplotlib/colorbar.py
Outdated
if (isinstance(self.norm, colors.LogNorm) | ||
and self._use_adjustable()): | ||
ax.set_xscale('log') | ||
ax.set_yscale('log') |
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.
and only the yscale be log 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.
They both need to be log to make the aspect
kwarg work properly (... or we could do a bunch of math and make the X values make the aspect ratio right.)
lib/matplotlib/colorbar.py
Outdated
|
||
self._set_label() | ||
|
||
def _get_ticker_locator_formatter(self): |
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.
This method could do with a quick comment underneath to explain what it does.
lib/matplotlib/colorbar.py
Outdated
else: | ||
b = self._boundaries[self._inside] | ||
locator = ticker.FixedLocator(b, nbins=10) | ||
# locator, formatter = _get_ticker_locator_formatter() |
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.
This commented out line can go
452e401
to
5972691
Compare
Pushed to 3.0 as this isn't urgent. But I think it'd be easy to put in 2.2 if someone wants to dive in and re-milestone... |
5972691
to
402b4d3
Compare
rebased |
The before-and-after comparison looks great, but tests seem to be failing now. |
Yeah, its just the streamplot image test at 0.036 tolerance. I'll try to track it down. It passes on my machine and I took the text out so it can't be freetype. Grrrr. |
I'm not sure that this is the right place to tell this, but I recently wrote a detailed instruction on colorbar in matplotlib which might contribute for improving tutorials about colorbar. After writing it, fortunately or not, I found this PR which seems to change colorbar's behavior significantly (no pseudo minor ticks, no normalization of vmin/vmax to 0/1, for example). Actually these modified behaviors are parts of the main topics in my instruction. Then I'm wondering whether I should translate the introduction as it is into English and make a PR to improve docs for the current behavior of colorbar (about cbar.ticker, especially), or I should wait for this PR to be implemented in, say, next rc version and revise the instruction to catch up the new behaviors. What do you think? |
@skotaro Sounds like we had some similar difficulties with colorbars. I will push in the near future for this PR to get in, but it won’t be part of the distribution until 3.0 (July/Aug). You may want to checkout constrained layout in 2.2Rc1 for easier colorbar placement. That doesn’t preclude a tutorial that explains how to fix things manually, but I’d suggest such a tutorial should at least reference the automated methods. |
The problem with the doc build is that after the merge, the sphinx conf.py |
651d609
to
2c33640
Compare
Somehow the doc build didn't like the name I had for the whats-new file. Works fine now. 🤷♂️ |
lib/matplotlib/colorbar.py
Outdated
ax.xaxis.get_major_formatter().set_offset_string(offset_string) | ||
_log.debug('Using fixed locator on colorbar') | ||
ticks, ticklabels, offset_string = self._ticker(locator, formatter) | ||
if self.orientation == 'vertical': |
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.
You can use the long_axis, short_axis here, too. Initialize them prior to the _use_auto_colorbar_locator
check.
Instead of ax.set_yticklabels
you can use the equivalent short_axis.set_ticklabels
, etc.
2c33640
to
252ef97
Compare
6bdc427
to
52e5d81
Compare
|
||
Colorbar ticks now adjust for the size of the colorbar if the | ||
colorbar is made from a mappable that is not a contour or | ||
doesn't have a BoundaryNorm, or boundaries are not specified. |
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.
extra space
lib/matplotlib/colorbar.py
Outdated
|
||
def __init__(self, colorbar): | ||
""" | ||
_ColorbarAutoLocator(colorbar) |
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.
Is there a particular need to include the signature?
lib/matplotlib/colorbar.py
Outdated
if type(self.norm) == colors.LogNorm: | ||
long_axis.set_minor_locator(_ColorbarLogLocator(self, | ||
base=10., subs='auto')) | ||
long_axis.set_minor_formatter(ticker.NullFormatter()) |
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.
Need to check that some ticks are labelled...
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.
Fixed. LogLocator works fine here.
@jklymak Is there an obvious reason why the ticks of the colorbar in the bottom right panel are not symmetrical (in log space) around 1 (e.g. [1e3, 1e0, 1e-3], instead of [1e2, 1e-1]), while the old one were “quite” symmetrical? |
@afvincent because the LogLocator only returned two tick marks as being able to fit and it chose those? Its just using the default LogLocator - i.e. if you had a normal yaxis that size, those are the ticks you'd get. |
@skotaro Are you still interested in including your (extensive!) colorbar tutorial into the mpl docs? Even if it needs to be updated following this PR and/or to take constrained_layout into account, feel free to open an early issue/PR to track the work. |
PR Summary
This PR changes colorbar to draw from
self.vmin
toself.vmax
instead of from 0 to 1 so thatAutoTickLocator
can work. Only modifies ticks if no boundaries are specified (i.e. contours).See #9246
example:
before:
after:
PR Checklist