Skip to content

Allow turning off minor ticks on Colorbar with LogNorm#13265

Merged
dstansby merged 4 commits into
matplotlib:masterfrom
LevN0:allow-disable-minorticks-lognorm-colorbar
Jan 23, 2019
Merged

Allow turning off minor ticks on Colorbar with LogNorm#13265
dstansby merged 4 commits into
matplotlib:masterfrom
LevN0:allow-disable-minorticks-lognorm-colorbar

Conversation

@LevN0

@LevN0 LevN0 commented Jan 22, 2019

Copy link
Copy Markdown
Contributor

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

jklymak commented Jan 23, 2019

Copy link
Copy Markdown
Member

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

jklymak commented Jan 23, 2019

Copy link
Copy Markdown
Member

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

Also allow minorticks_on for LogNorm
@LevN0

LevN0 commented Jan 23, 2019

Copy link
Copy Markdown
Contributor Author

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

jklymak commented Jan 23, 2019

Copy link
Copy Markdown
Member

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

jklymak commented Jan 23, 2019

Copy link
Copy Markdown
Member

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

NelleV commented Jan 23, 2019

Copy link
Copy Markdown
Member

This looks good! Can you add a unit test?

@LevN0

LevN0 commented Jan 23, 2019

Copy link
Copy Markdown
Contributor Author

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

@jklymak jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @LevN0 !

@dstansby dstansby left a comment

Copy link
Copy Markdown
Member

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

LevN0 commented Jan 23, 2019

Copy link
Copy Markdown
Contributor Author

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