Skip to content

Allow turning off minor ticks on Colorbar with LogNorm #13265

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

Conversation

LevN0
Copy link
Contributor

@LevN0 LevN0 commented Jan 22, 2019

Allow turning off minor ticks on Colorbar with LogNorm

PR Summary

Closes #13257.

Allows disabling minor ticks for Colorbar when it has LogNorm.

Before

Minor ticks can be disabled for all norms except LogNorm.

The following example is intended to show that when having arbitrary major tick marks, the minor tick marks add rather than remove confusion. Example,

fig, ax = plt.subplots()

data = np.clip(randn(250, 250)*1000, 0, 1000)

cax = ax.imshow(data, norm=mpl.colors.LogNorm(vmin=1, vmax=1000))
cbar = fig.colorbar(cax, format='%d', ticks=[5, 50, 500])

Produces,

After

Once minor ticks can be disabled even for LogNorm, calling cbar.minorticks_off() will leave only major ticks in the above figure.

Allow minorticks back on for LogNorm as well

Since minor ticks can now be turned off for LogNorm, makes sense to allow turning them back on for LogNorm too.

Previously they would be enabled in ColorbarBase.update_ticks for LogNorm by default, however minorticks_off and minorticks_off was in Colorbar. These have been moved, in part so that update_ticks can take advantage of them.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 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

Allow turning off minor ticks on Colorbar with LogNorm
@jklymak
Copy link
Member

jklymak commented Jan 23, 2019

Looks good! Can you also remove the check that prevents turning them back on?

Does anyone know why we did this in the first place?

@jklymak
Copy link
Member

jklymak commented Jan 23, 2019

#11584 (comment) Looks to just have been a misunderstanding....

Also allow minorticks_on for LogNorm
@LevN0
Copy link
Contributor Author

LevN0 commented Jan 23, 2019

Uh oh, this is probably going to be more complicated than I thought.

It makes sense to remove the check that prevents turning ticks back on with LogNorm, but then it still needs an else. I am not sure how to do it right. Do not know why minor ticks are enabled in ColorbarBase for LogNorm but for everything else the methods are in Colorbar.

@jklymak
Copy link
Member

jklymak commented Jan 23, 2019

Do not know why minor ticks are enabled in ColorbarBase for LogNorm but for everything else the methods are in Colorbar.

That might just be a mistake?

@jklymak
Copy link
Member

jklymak commented Jan 23, 2019

I actually think the minor tick methods should go on ColorbarBase. The only reason for Colorbar (I think) is to add a mappable to the the colorbar.

PEP8
@NelleV
Copy link
Member

NelleV commented Jan 23, 2019

This looks good! Can you add a unit test?

@LevN0
Copy link
Contributor Author

LevN0 commented Jan 23, 2019

Made an attempt.

Edit: I really should have just setup the environment to run pytest locally, would have saved me time in seeing the miss-named import.

Add unit tests
@LevN0 LevN0 force-pushed the allow-disable-minorticks-lognorm-colorbar branch from 8899bae to eb411d0 Compare January 23, 2019 05:56
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @LevN0 !

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.

Thanks!

@dstansby dstansby added this to the v3.1 milestone Jan 23, 2019
@dstansby dstansby merged commit 397e79e into matplotlib:master Jan 23, 2019
@LevN0
Copy link
Contributor Author

LevN0 commented Jan 23, 2019

Thank you everyone for the guidance, especially @jklymak.

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.

4 participants