Skip to content

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

Merged
merged 2 commits into from
Mar 22, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Dec 1, 2017

PR Summary

This PR changes colorbar to draw from self.vmin to self.vmax instead of from 0 to 1 so that AutoTickLocator can work. Only modifies ticks if no boundaries are specified (i.e. contours).

See #9246

example:

import matplotlib.pyplot as plt
import matplotlib.colors as mcolors
import numpy as np

X = np.random.randn(32, 32)


fig, ax = plt.subplots(2, 2, figsize=(5, 5), tight_layout=True)
for nn, shrink in enumerate([1., 0.4]):
    for mm, log in enumerate([False, True]):
        if log:
            norm = mcolors.LogNorm()
            Y = 10**X
        else:
            norm = None
            Y = X
        pc = ax[nn, mm].pcolormesh(Y, norm=norm)
        ax[nn, mm].set_xticks([])
        ax[nn, mm].set_yticks([])
        fig.colorbar(pc, ax=ax[nn,mm], shrink=shrink, extend='both')

plt.show()
fig.savefig('ExampleCBOld.png')

before:

examplecbold

after:

examplecbnew

PR Checklist

  • Test/get to work LogNorm
  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell tacaswell added this to the v2.2 milestone Dec 1, 2017
@jklymak
Copy link
Member Author

jklymak commented Dec 1, 2017

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.

@jklymak jklymak force-pushed the enh-colorbar-ticks branch 2 times, most recently from 9d1146d to 5c48c10 Compare December 2, 2017 06:48
@jklymak jklymak changed the title WIP: ENH: adjustable colorbar ticks ENH: adjustable colorbar ticks Dec 2, 2017
@jklymak
Copy link
Member Author

jklymak commented Dec 2, 2017

This is redone, and much simpler now. 4 image files had to be changed...

@jklymak
Copy link
Member Author

jklymak commented Dec 19, 2017

Ping @efiring LogNorm() now works. I'm too lazy to do SymLogNorm() (well actually I disapprove of that normalization as a valid thing to do to data), but someone could easily figure it out.

The diff looks worse than it is because I moved a bit of code around.


self._set_label()

def _get_ticker_locator_formatter(self):
Copy link
Member Author

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

Copy link
Member

@dstansby dstansby left a 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

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".
Copy link
Member

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?

extrude into the "extend regions".
"""

def __init__(self, colorbar, *args, **kwargs):
Copy link
Member

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):
Copy link
Member

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__

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()
Copy link
Member

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?

if (isinstance(self.norm, colors.LogNorm)
and self._use_adjustable()):
ax.set_xscale('log')
ax.set_yscale('log')
Copy link
Member

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?

Copy link
Member Author

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


self._set_label()

def _get_ticker_locator_formatter(self):
Copy link
Member

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.

else:
b = self._boundaries[self._inside]
locator = ticker.FixedLocator(b, nbins=10)
# locator, formatter = _get_ticker_locator_formatter()
Copy link
Member

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

@dstansby dstansby self-assigned this Dec 24, 2017
@jklymak jklymak requested review from efiring and removed request for efiring January 10, 2018 03:38
@jklymak jklymak modified the milestones: v2.2, v3.0 Jan 10, 2018
@jklymak
Copy link
Member Author

jklymak commented Jan 10, 2018

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

@jklymak
Copy link
Member Author

jklymak commented Feb 4, 2018

rebased

@jklymak
Copy link
Member Author

jklymak commented Feb 4, 2018

@dstansby I think I addressed all your concerns.
@efiring requesting a review from you (though no rush) as this is your bailiwick...

@jklymak jklymak requested a review from efiring February 4, 2018 18:18
@dstansby dstansby dismissed their stale review February 4, 2018 19:12

Old review

@efiring
Copy link
Member

efiring commented Feb 5, 2018

The before-and-after comparison looks great, but tests seem to be failing now.

@jklymak
Copy link
Member Author

jklymak commented Feb 5, 2018

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.

@skotaro
Copy link

skotaro commented Feb 20, 2018

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.
https://qiita.com/skotaro/items/01d66a8c9902a766a2c0
It's only in Japanese for the moment but I think you can get the ideas from figures and sample codes. Main topics are 1. Anatomy of a colorbar, 2. Putting a colorbar at the right place with the right size, and 3. Changing ticker into log scale nicely. It's not comprehensive but I think it still helps users puzzled by 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?

@jklymak
Copy link
Member Author

jklymak commented Feb 20, 2018

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

@efiring
Copy link
Member

efiring commented Mar 18, 2018

The problem with the doc build is that after the merge, the sphinx conf.py exclude_patterns list is out of sync with the location where you are putting your users/whats_new file.
It looks like this is a bug in master: there are directories users/prev_whats_new and users/next_whats_new, but exclude_patterns = ['api/api_changes/*', 'users/whats_new/*'].

@jklymak jklymak force-pushed the enh-colorbar-ticks branch 5 times, most recently from 651d609 to 2c33640 Compare March 18, 2018 04:58
@jklymak
Copy link
Member Author

jklymak commented Mar 18, 2018

Somehow the doc build didn't like the name I had for the whats-new file. Works fine now. 🤷‍♂️

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':
Copy link
Member

@efiring efiring Mar 18, 2018

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.

@jklymak jklymak force-pushed the enh-colorbar-ticks branch from 2c33640 to 252ef97 Compare March 18, 2018 15:23
@jklymak jklymak force-pushed the enh-colorbar-ticks branch from 6bdc427 to 52e5d81 Compare March 18, 2018 16:01

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space


def __init__(self, colorbar):
"""
_ColorbarAutoLocator(colorbar)
Copy link
Contributor

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?

if type(self.norm) == colors.LogNorm:
long_axis.set_minor_locator(_ColorbarLogLocator(self,
base=10., subs='auto'))
long_axis.set_minor_formatter(ticker.NullFormatter())
Copy link
Member Author

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

Copy link
Member Author

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.

@afvincent
Copy link
Contributor

@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?

@jklymak
Copy link
Member Author

jklymak commented Mar 21, 2018

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

@anntzer
Copy link
Contributor

anntzer commented Oct 12, 2018

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

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

Successfully merging this pull request may close these issues.

9 participants