FIX: fix colorbars with no scales #20327
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jklymak
added
the
Release critical
jklymak
force-pushed
the
fix-colorbars-with-no-scale
branch
2 times, most recently
from
2177d8c
to
52a6aa8
Compare
jklymak
commented
May 29, 2021
| @@ -1152,7 +1152,7 @@ def __init__(self, vmin=None, vmax=None, clip=False): | |||
| self.vmin = _sanitize_extrema(vmin) | |||
| self.vmax = _sanitize_extrema(vmax) | |||
| self.clip = clip | |||
| self._scale = scale.LinearScale(axis=None) | |||
| self._scale = None # will default to LinearScale for colorbar | |||
There was a problem hiding this comment.
Note downstream libraries will likely derive from Normalize, so scale should be None so their norm gets used...
There was a problem hiding this comment.
@dstansby can you see if this version works?
There was a problem hiding this comment.
Yep, seems to work well with this version.
There was a problem hiding this comment.
Any chance of a formal review?
jklymak
force-pushed
the
fix-colorbars-with-no-scale
branch
from
7867fc3
to
1e8658d
Compare
greglucas
reviewed
May 31, 2021
| @image_comparison(['colorbar_twoslope.png'], remove_text=True, | ||
| style='mpl20') | ||
| def test_twoslope_colorbar(): | ||
| # Note that the first tick = 20, and should be in the middle |
There was a problem hiding this comment.
Adding the ticklabels to the image would be helpful here.
There was a problem hiding this comment.
Yeah, I agree, but we really try to avoid labels, because they cause image comparison problems when the font library changes...
QuLogic
reviewed
Jun 1, 2021
7 tasks
jklymak
force-pushed
the
fix-colorbars-with-no-scale
branch
from
87b8ae7
to
d1c5a6a
Compare
QuLogic
approved these changes
Jun 10, 2021
anntzer
approved these changes
Jun 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary
Closes #20324
The colorbar re-write #20054, needs a scale for the colorbar. Which worked great for most of our Norms because they are all created from a scale now:
matplotlib/lib/matplotlib/colors.py
Line 1435 in 08f4629
However, astropy did not derive their norms from a scale, so they had a failure (which I hope this fixes). astropy/astropy#11800
We also had norms like
TwoSlopeNormthat do not derive from a scale, though they probably should/could.Here the proposal is that if there is no scale on the norm, the scale is a
functionscale with the forward and inverse from the norm. This works fine forTwoSlopeNorm(ahem, after we define the inverse for it, which really should have been done when it went in; that took a bit of work to realize).PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).