FIX: colorbar contour with log norm should default to log locator and formatter... by jklymak · Pull Request #23390 · matplotlib/matplotlib · GitHub

PR Summary

This sets the colorbar to have a log scale if the norm on contours is log. This wasn't previously tested, but did break an example. Without this fix, the contours get a linear ticker at the contour boundaries.

closes #23389

PR Checklist

Tests and Styling

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

... there is a workaround, so perhaps not release critical.

It seems odd that this special casing has to be done, what's stopping the more generic code path on L1228 being hit below? (perhaps LogNorm doesn't have a _scale attribute set?)

The contours have boundaries so we get caught in the first case which just uses a linear scale.

Should that first case be modified to get the scale from self.norm._scale (if present) instead then?

Yes, thanks, I think you are right. Lets see if this passes the other tests...

Yes, this works, and I think is more consistent with what users would want for other boundary norm cases. Thanks!

This now seems like you are special-casing LogNorms only for contoursets.

I think the main issue is that there are some self.boundaries, but the norm is changed so we don't want to take that path, so perhaps we could add a check on the norm below.

(self.boundaries is not None  and type(self.norm) is colors.Normalize)

What I'm worried about is that we are whacking the LogLocator() with this change, but we also want to be able to handle any other special-cased locators too without adding additional if clauses above.

It may be valid, but I'm not following your concern here. This is usually only called at init, or if the norm is changed, both of which are pretty destructive to the colorbar. Everything gets whacked, as noted in the docstring.

My concern is mostly that this handles: locator=ticker.LogLocator(), and will this mean that we'll also need to special case SymmetricalLogLocator() in the future.

But, looking at the contour code it looks like only log-scales are special cased, and they set an explicit logscaled attribute. So, should we actually be using that here instead?

if getattr(self.mappable, "logscale", False):

This doesn't now seem to fix the original issue? I don't see minor ticks with ec8ae4b as you do with cbar.ax.set_yscale('log').

the original issue was that the formatter was the normal formatter, not the scientific formatter.

The other thing that changed is that before colorbar had its own ticking and handled log ticking differently than normal axes. Now the ticker is the same as for any other axes, and for this dynamic range there are no minor ticks drawn. I think making the colorbar consistent with other axes should trump the small breakage here, rather than trying to have a special mechanism to tick colorbars differently.

Maybe do this as an svg test and keep the text? The issue is the labels are wrong not the tick locations.

The original issue looks like this on main:

and like this on the final commit of this PR:

Whereas, the second-last commit looks like this:

So with the final fix as it stands, it seems to do nothing, and the previous attempt was better.

Calling cbar = fig.colorbar(cs, spacing='proportional') on the last commit also get the log-formatted tick labels.

Ooops, yes, I see. I thought proportional was the default, but its actually uniform... This is pretty clearly a case where we have gone overboard in the API having a lot of edge cases. I'll try to give it some thought to how to fix, and indeed maybe going back to the previous fix...

OK, reverted back to the case that was working before with the hard-coded check. Also updated test to use svg with text.

Hard cycled to get a retry on circle CI

This does fix the example, so approving as an improvement for now. If someone hits some other edge cases it can be fixed in a follow-up PR.

My concern is mostly that this handles: locator=ticker.LogLocator(), and will this mean that we'll also need to special case SymmetricalLogLocator() in the future.

But, looking at the contour code it looks like only log-scales are special cased, and they set an explicit logscaled attribute. So, should we actually be using that here instead?

if getattr(self.mappable, "logscale", False):

I think the Circle CI failure may be fixed if you rebase this on upstream/main to bring it up to date with main because the failures look unrelated to this PR.

I don't know that this needs to be back ported to 3.6, but it would be nice to get in for 3.7

It seems to have two approvals, so...

…uld default to log locator and formatter...

…390-on-v3.7.x Backport PR #23390 on branch v3.7.x (FIX: colorbar contour with log norm should default to log locator and formatter...)

…390-on-v3.6.x Backport PR #23390 on branch v3.6.x (FIX: colorbar contour with log norm should default to log locator and formatter...)

Hello, is this issue really fixed?
Doing cbar = fig.colorbar(cs, spacing='proportional') add another colorbar which is wrong
Doing cbar.ax.set_yscale('log') leaves white spaces in the colorbar in the upper and lower ends

@jmorenoven2 Please open a new issue with a reproducible example of the problem you are having.

Calling fig.colorbar more than once is expected to create multiple colorbars and without context it is very hard to understand what setting the scale on colorbar yaxis should be doing.

jklymak added the topic: color/colorbar label Jul 5, 2022

jklymak force-pushed the fix-colorbar-contour-log branch from 9a9499c to fda4e5f Compare July 6, 2022 07:48

QuLogic reviewed Jul 6, 2022

View reviewed changes

Comment thread lib/matplotlib/tests/test_contour.py Outdated

jklymak force-pushed the fix-colorbar-contour-log branch from fda4e5f to 907c109 Compare July 6, 2022 09:23

jklymak marked this pull request as ready for review July 6, 2022 09:51

jklymak added this to the v3.5.3 milestone Jul 6, 2022

dstansby reviewed Jul 7, 2022

View reviewed changes

jklymak force-pushed the fix-colorbar-contour-log branch from c80563a to ec8ae4b Compare July 7, 2022 20:01

dstansby self-requested a review July 7, 2022 20:15

dstansby approved these changes Jul 8, 2022

View reviewed changes

tacaswell reviewed Jul 26, 2022

View reviewed changes

jklymak marked this pull request as draft July 26, 2022 20:57

tacaswell modified the milestones: v3.5.3, v3.5.4 Aug 3, 2022

oscargus modified the milestones: v3.5.4, v3.6.0 Aug 19, 2022

QuLogic modified the milestones: v3.6.0, v3.6.1 Sep 9, 2022

jklymak mentioned this pull request Sep 25, 2022

[Bug]: Matplotlib >= 3.5.0 drops scientific notation for colorbars using log ticks #24002

Closed

QuLogic modified the milestones: v3.6.1, v3.6.2 Oct 6, 2022

QuLogic modified the milestones: v3.6.2, v3.6.3 Oct 27, 2022

jklymak force-pushed the fix-colorbar-contour-log branch from ec8ae4b to 907c109 Compare December 11, 2022 21:59

jklymak force-pushed the fix-colorbar-contour-log branch 2 times, most recently from 2311f53 to 7f4522e Compare December 11, 2022 22:21

jklymak marked this pull request as ready for review December 11, 2022 22:22

jklymak closed this Dec 12, 2022

jklymak reopened this Dec 12, 2022

greglucas approved these changes Dec 13, 2022

View reviewed changes

FIX: make colorbars for log normed contour plots have logscale

c06f3bd

jklymak force-pushed the fix-colorbar-contour-log branch from 7f4522e to c06f3bd Compare December 15, 2022 21:38

QuLogic merged commit 1bb68a8 into matplotlib:main Jan 11, 2023

meeseeksmachine mentioned this pull request Jan 11, 2023

Backport PR #23390 on branch v3.7.x (FIX: colorbar contour with log norm should default to log locator and formatter...) #24938

Merged

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jan 11, 2023

Backport PR matplotlib#23390 : FIX: colorbar contour with log norm sho…

a3a66a9

…uld default to log locator and formatter...

meeseeksmachine mentioned this pull request Jan 11, 2023

Backport PR #23390 on branch v3.6.x (FIX: colorbar contour with log norm should default to log locator and formatter...) #24939

Merged

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jan 11, 2023

Backport PR matplotlib#23390 : FIX: colorbar contour with log norm sho…

88a1537

…uld default to log locator and formatter...

QuLogic added a commit that referenced this pull request Jan 11, 2023

Merge pull request #24938 from meeseeksmachine/auto-backport-of-pr-23…

1fb4d50

…390-on-v3.7.x Backport PR #23390 on branch v3.7.x (FIX: colorbar contour with log norm should default to log locator and formatter...)

QuLogic added a commit that referenced this pull request Jan 11, 2023

Merge pull request #24939 from meeseeksmachine/auto-backport-of-pr-23…

4f30cf4

…390-on-v3.6.x Backport PR #23390 on branch v3.6.x (FIX: colorbar contour with log norm should default to log locator and formatter...)

ksunden mentioned this pull request Feb 20, 2023

merge up 3.7.0 #25270

Merged

6 tasks

		if (isinstance(self.mappable, contour.ContourSet) and
		isinstance(self.norm, colors.LogNorm)):

		plt.colorbar(c, ax=ax)


		@image_comparison(['contour_log_locator.png'], style='mpl20', remove_text=True)

FIX: colorbar contour with log norm should default to log locator and formatter... by jklymak · Pull Request #23390 · matplotlib/matplotlib · GitHub

Repository navigation

FIX: colorbar contour with log norm should default to log locator and formatter... - #23390

FIX: colorbar contour with log norm should default to log locator and formatter...#23390QuLogic merged 1 commit intomatplotlib:mainmatplotlib/matplotlib:mainfrom jklymak:fix-colorbar-contour-logjklymak/matplotlib:fix-colorbar-contour-logCopy head branch name to clipboard

Conversation

jklymak commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Uh oh!

jklymak commented Jul 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Jul 8, 2022

Uh oh!

jklymak commented Jul 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Jul 26, 2022

Uh oh!

tacaswell commented Jul 26, 2022

Uh oh!

jklymak commented Jul 26, 2022

Uh oh!

jklymak commented Dec 11, 2022

Uh oh!

jklymak commented Dec 12, 2022

Uh oh!

greglucas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglucas commented Dec 13, 2022

Uh oh!

jklymak commented Jan 10, 2023

Uh oh!

QuLogic commented Jan 11, 2023

Uh oh!

jmorenoven2 commented Feb 6, 2023

Uh oh!

tacaswell commented Feb 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

FIX: colorbar contour with log norm should default to log locator and formatter...#23390
QuLogic merged 1 commit into
matplotlib:mainfrom
jklymak:fix-colorbar-contour-log

jklymak commented Jul 5, 2022 •
edited
Loading