Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
FIX: colorbar contour with log norm should default to log locator and formatter...#23390
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
9a9499c tofda4e5fCompareUh oh!
There was an error while loading.Please reload this page.
fda4e5f to907c109Compare... there is a workaround, so perhaps not release critical. |
| if (isinstance(self.mappable, contour.ContourSet) and | ||
| isinstance(self.norm, colors.LogNorm)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It seems odd that this special casing has to be done, what's stopping the more generic code path on L1228 being hit below? (perhapsLogNorm doesn't have a_scale attribute set?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The contours have boundaries so we get caught in the first case which just uses a linear scale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should that first case be modified to get the scale fromself.norm._scale (if present) instead then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, thanks, I think you are right. Lets see if this passes the other tests...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this works, and I think is more consistent with what users would want for other boundary norm cases. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This now seems like you are special-casing LogNorms only for contoursets.
I think the main issue is that there are someself.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.boundariesisnotNoneandtype(self.norm)iscolors.Normalize)
What I'm worried about is that we are whacking theLogLocator() with this change, but we also want to be able to handle any other special-cased locators too without adding additional if clauses above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My concern is mostly that this handles:locator=ticker.LogLocator(), and will this mean that we'll also need to special caseSymmetricalLogLocator() in the future.
But, looking at the contour code it looks like only log-scales are special cased, and they set an explicitlogscaled attribute. So, should we actually be using that here instead?
if getattr(self.mappable, "logscale", False):
c80563a toec8ae4bCompareThis doesn't now seem to fix the original issue? I don't see minor ticks withec8ae4b as you do with |
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. |
lib/matplotlib/tests/test_contour.py Outdated
| @@ -154,6 +154,27 @@ def test_given_colors_levels_and_extends(): | |||
| plt.colorbar(c, ax=ax) | |||
| @image_comparison(['contour_log_locator.png'], style='mpl20', remove_text=True) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 So with the final fix as it stands, it seems to do nothing, and the previous attempt was better. |
Calling |
Ooops, yes, I see. I thought |
ec8ae4b to907c109Compare2311f53 to7f4522eCompareOK, 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| if (isinstance(self.mappable, contour.ContourSet) and | ||
| isinstance(self.norm, colors.LogNorm)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My concern is mostly that this handles:locator=ticker.LogLocator(), and will this mean that we'll also need to special caseSymmetricalLogLocator() in the future.
But, looking at the contour code it looks like only log-scales are special cased, and they set an explicitlogscaled 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 |
7f4522e toc06f3bdCompareI 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...
…uld default to log locator and formatter...
…390-on-v3.7.xBackport PR#23390 on branch v3.7.x (FIX: colorbar contour with log norm should default to log locator and formatter...)
…390-on-v3.6.xBackport PR#23390 on branch v3.6.x (FIX: colorbar contour with log norm should default to log locator and formatter...)
jmorenoven2 commentedFeb 6, 2023
Hello, is this issue really fixed? |
@jmorenoven2 Please open a new issue with a reproducible example of the problem you are having. Calling |
Uh oh!
There was an error while loading.Please reload this page.
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
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).