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

[jklymak]

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

[jklymak]

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

[dstansby]

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

[jklymak]

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

[dstansby]

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

[jklymak]

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

[jklymak]

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

[greglucas]

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.

[jklymak]

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.

[greglucas]

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

[QuLogic]

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

[jklymak]

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.

[tacaswell]

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

[QuLogic]

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.

[tacaswell]

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

[jklymak]

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

[jklymak]

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

[jklymak]

Hard cycled to get a retry on circle CI

[greglucas]

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.

[greglucas]

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

[greglucas]

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.

[jklymak]

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

[QuLogic]

It seems to have two approvals, so...

[jmorenoven2]

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

[tacaswell]

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