ENH: fix colorbar bad minor ticks #11584

pharshalp · 2018-07-06T03:08:13Z

PR Summary

This follows the suggestion given in #11556 and builds a method for colorbar to correctly display minor ticks (without extending into the region beyond vmin, vmax when the extend option is used).

Earlier, the only way to display the minor ticks was to use ax.minorticks_on/off methods
(which didn't respect vmin, vmax values).

With this PR, the colorbar gets its own minorticks_on/off methods.

Fixes #11510 and is a follow up to #11556 .

@jklymak Could you please review this?
This builds on top of your suggestions in the discussion here #11556, #11510.

This is my first (relatively significant) contribution to matplotlib (apart from a minor documentation entry) so I would appreciate any help in how to properly document API change (or if I am missing anything else). ~~I am marking this as WIP since I am not sure how to add tests and other entries from the PR checklist (I will try to understand the requirements for each item in the list and will work on those soon)~~.

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc 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~~

Correct behavior (after the PR)

import matplotlib.pyplot as plt
import numpy as np

# Making yticks longer in length to highlight the issue
plt.rcParams['ytick.major.size'] = 10
plt.rcParams['ytick.minor.size'] = 4


np.random.seed(seed=12345)
x = np.random.randn(20, 20)
fig, ax = plt.subplots()
im = ax.pcolormesh(x)

cbar = fig.colorbar(im, extend='both')
###  Colorbar gets its own minorticks_on() method
cbar.minorticks_on()
plt.show()

Incorrect behavior

import matplotlib.pyplot as plt
import numpy as np

# Making yticks longer in length to highlight the issue
plt.rcParams['ytick.major.size'] = 10
plt.rcParams['ytick.minor.size'] = 4


np.random.seed(seed=12345)
x = np.random.randn(20, 20)
fig, ax = plt.subplots()
im = ax.pcolormesh(x)

cbar = fig.colorbar(im, extend='both')

###  older way of adding minorticks (which had incorrect behavior)
cbar.ax.minorticks_on()
plt.show()

afvincent · 2018-07-06T03:29:17Z

@pharshalp Thanks for the PR :)!

If you have not already seen it, there are a few informations about writing tests here in the documentation. FWIW, the current trend in Matplotlib is to avoid “image tests” as much as possible (mostly to keep the size of git history reasonable). Here, one could for example “simply” test that the tick values are the expected ones, rather than comparing to a fully pre-rendered reference picture. (Such a test could be added to lib/matplotlib/tests/test_colorbar.py.)

afvincent · 2018-07-06T03:31:32Z

lib/matplotlib/colorbar.py

+        locs = locs.compress(cond)
+
+        return self.raise_if_exceeds(np.array(locs))
+
 class _ColorbarLogLocator(ticker.LogLocator):


The CI will likely complain if there are not 2 blank lines before a new class definition.

Thanks! just pushed a fix.

jklymak · 2018-07-06T04:12:04Z

This looks like a great start. Will definitely need a test! Have a look in lib/matplotlib/tests/test_colorbar.py, and perhaps add or modify
test_colorbar_autoticks()? Thanks a lot for doing this.

pharshalp · 2018-07-06T04:22:28Z

added a test that checks for the correct behavior of cbar.minorticks_on and cbar.minorticks_off().

jklymak · 2018-07-06T04:32:50Z

lib/matplotlib/colorbar.py

+        self.vmax = self._colorbar.norm.vmax
+        self.ndivs = n
+
+    def __call__(self):


Can't you just call ticker.AutoMinorLocator and then trim the values?

~~ticker.AutoMinorLocator doesn't seem to have a tick_values method that would return the tick locations.~~

def tick_values(self, vmin, vmax): raise NotImplementedError('Cannot get tick locations for a ' '%s type.' % type(self))

Instead, AutoMinorLocatior has a method called call which returns the tick values... (the calculation for the locations of the minor ticks is done inside call which I have copied and modified).

def __call__(self): 'Return the locations of the ticks'

~~Please let me know if I am missing a better way of doing this. Thanks!~~

ahh I see what you meant there... will fix it.

Thanks @jklymak I have pushed a cleaner version now.

jklymak

This looks almost done, and I think the code is all correct. Nice test.

I think it needs a "whats new" entry because I don't think colorbars had minorticks_on/off methods before. I think its a reasonable addition, though I guess the "old" way to do this would have been to simply act on the colorbar's axes directly.

Please add an entry to doc/users/next_whats_new following one of the templates there (don't forget to git add the new file before committing)!

Thanks again, this is very nice...

pharshalp · 2018-07-06T18:36:00Z

Thank you @jklymak for your prompt help in reviewing this. I have added a 'whats new' entry.

Since technically this is not a new feature (we are just fixing an incorrect behavior), I am not sure if the "New features are documented, with examples if plot related" rule applies here.
Also, should I go ahead and squash all the commits into one?

jklymak · 2018-07-06T18:54:20Z

But the method to add minor ticks to colorbar is new, isn’t it?

Yes squashing is preferred.

Thanks

jklymak · 2018-07-06T20:17:48Z

lib/matplotlib/colorbar.py

+        else:
+            long_axis = ax.xaxis
+
+        long_axis.set_minor_locator(_ColorbarAutoMinorLocator(self))


Sorry to review in bits and drabs...

What happens if the colorbar is logarithmic? Is this function still a good idea? Or should you be setting a _ColorbarLogMinorLocator?

you are right... I completely overlooked that case. Will fix it.

hmm... this is an interesting journey! From what I understand, LogLocator only returns Major ticks and AutoMinorLocator doesn't work with log scale... I guess I will have to dig deeper.

I think the log locator looks after the minor ticks, but I could be misremembering. So in this case, you probably just need to be careful that "minorticks_on" just doesn't do anything so as not to step on the log locator's toes.

made sure that minorticks_on and minorticks_off doesn't do anything when called on a logarithmic colorbar axis (also added a warning letting the user know about this).

pharshalp · 2018-07-07T03:05:55Z

Modified an existing example to show off the utility of cbar.minorticks_on()

https://matplotlib.org/gallery/color/colorbar_basics.html#sphx-glr-gallery-color-colorbar-basics-py

How can I check that the "Documentation is sphinx and numpydoc compliant"?. I do not have any experience with sphinx, numpydoc.

jklymak

Not blocking on my gripes on the example. Its functional, just not very good to look at. Otherwise, I think this is very helpful and well done. Thanks!

pharshalp · 2018-07-07T04:14:39Z

Thank you @jklymak for your guidance and for reviewing this. Thanks to @afvincent too!

timhoffm

Thanks. A well written PR. Only some minor comments.

timhoffm · 2018-07-07T10:40:09Z

doc/users/next_whats_new/colorbar_minor_ticks.rst

+Add ``minorticks_on()/off()`` methods for colorbar
+--------------------------------------------------
+
+A new method :meth:`~matplotlib.colorbar.Colobar.minorticks_on` is


You could use `.Colorbar.minorticks_on`

thanks... I wasn't really familiar with this style of documentation so I was just following what I saw in some of the other whats new files :D

timhoffm · 2018-07-07T10:40:23Z

doc/users/next_whats_new/colorbar_minor_ticks.rst

+doesn't allow the minor ticks to extend into the regions beyond vmin and vmax
+when the extend `kwarg` (used while creating the colorbar) is set to 'both',
+'max' or 'min'.
+A complementary method :meth:`~matplotlib.colorbar.Colobar.minorticks_off`


`.Colorbar.minorticks_off`

timhoffm · 2018-07-07T10:42:00Z

lib/matplotlib/colorbar.py

+        This ticker needs to know the *colorbar* so that it can access
+        its *vmin* and *vmax*.
+        """
+


no need for empty line after docstrings

timhoffm · 2018-07-07T10:45:00Z

lib/matplotlib/colorbar.py

+        if self.orientation == 'vertical':
+            long_axis = ax.yaxis
+        else:
+            long_axis = ax.xaxis


more pythonic:

long_axis = ax.yaxis if self.orientation == 'vertical' else ax.xaxis

timhoffm · 2018-07-07T10:49:32Z

lib/matplotlib/colorbar.py

+        """
+        try:
+            ax = self.ax
+        except AttributeError:


Can that happen? The attribute is defined in ColorbarBase.__init__.

You are right... removed the try except block

timhoffm · 2018-07-07T11:01:19Z

lib/matplotlib/tests/test_colorbar.py

@@ -270,6 +270,31 @@ def test_colorbar_ticks():
    assert len(cbar.ax.xaxis.get_ticklocs()) == len(clevs)


+def test_colorbar_minorticks_on_off():
+    # test for #11584


# test for github issue #11584

pharshalp · 2018-07-07T13:44:18Z

@timhoffm Thanks! addressed your suggestions.

@jklymak

Added a missing blank line before class definition added a test for minorticks_on/off Renamed the test for colorbar minorticks_on/off and added helpful comment Cleaned up the __call__ method for _ColorbarAutoMinorLocator as per the suggestion of @jklymak Added whats new entry making sure that minorticks_on/off doesn't act on a logarithmic colorbar axis Added _ColorbarAutoMinorLocator, minorticks_on/off, test_colorbar_minorticks_on_off and whats new entry Added an example showing off the utility of cbar.minorticks_on() Cleaning up

timhoffm

Thanks! Anyone can merge after CI pass.

pharshalp · 2018-07-08T01:00:41Z

I hope this doesn't come across as being pushy but, I was wondering if this will make it to 3.0 milestone (or if the 3.0 milestone is reserved only for critical fixes).

jklymak · 2018-07-08T01:05:36Z

If its in master when 3.0 is branched (which hasn't happened yet so far as I know), then it'll be in 3.0..

afvincent added the status: work in progress label Jul 6, 2018

afvincent reviewed Jul 6, 2018

View reviewed changes

afvincent requested a review from jklymak July 6, 2018 03:34

jklymak reviewed Jul 6, 2018

View reviewed changes

pharshalp changed the title ~~WIP: ENH: fix colorbar bad minor ticks~~ ENH: fix colorbar bad minor ticks Jul 7, 2018

jklymak approved these changes Jul 7, 2018

View reviewed changes

timhoffm reviewed Jul 7, 2018

View reviewed changes

timhoffm approved these changes Jul 7, 2018

View reviewed changes

jklymak merged commit 5211ed8 into matplotlib:master Jul 7, 2018

pharshalp deleted the fix-colorbar-minor-ticks branch July 7, 2018 21:48

jklymak removed the status: work in progress label Jul 8, 2018

QuLogic added this to the v3.0 milestone Jul 8, 2018

This was referenced Sep 15, 2018

Mention colorbar.minorticks_on/off in references #12130

Merged

FIX: make sure all ticks show up for colorbar minor tick #12099

Merged

jklymak mentioned this pull request Jan 23, 2019

Allow turning off minor ticks on Colorbar with LogNorm #13265

Merged

6 tasks

pharshalp mentioned this pull request Feb 11, 2020

incorrect cbar minor ticks for extend regions when x/ytick.minor.visible is True #16468

Closed

ENH: fix colorbar bad minor ticks #11584

ENH: fix colorbar bad minor ticks #11584

Conversation

pharshalp commented Jul 6, 2018 • edited Loading

PR Summary

PR Checklist

afvincent commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Jul 6, 2018

pharshalp commented Jul 6, 2018

Choose a reason for hiding this comment

pharshalp Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

pharshalp commented Jul 6, 2018

jklymak commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pharshalp Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pharshalp commented Jul 7, 2018 • edited Loading

jklymak left a comment

Choose a reason for hiding this comment

pharshalp commented Jul 7, 2018

timhoffm left a 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

pharshalp commented Jul 7, 2018

timhoffm left a comment

Choose a reason for hiding this comment

pharshalp commented Jul 8, 2018 • edited Loading

jklymak commented Jul 8, 2018

pharshalp commented Jul 6, 2018 •

edited

Loading

pharshalp Jul 6, 2018 •

edited

Loading

pharshalp Jul 6, 2018 •

edited

Loading

pharshalp commented Jul 7, 2018 •

edited

Loading

pharshalp commented Jul 8, 2018 •

edited

Loading