-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix cbars for different norms #16286
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Obviously the test is a good idea! Not sure what to do about the failure though ;-) |
@@ -570,3 +570,17 @@ def test_colorbar_label(): | |||
|
|||
cbar3 = fig.colorbar(im, orientation='horizontal', label='horizontal cbar') | |||
assert cbar3.ax.get_xlabel() == 'horizontal cbar' | |||
|
|||
@image_comparison( | |||
baseline_images=['colorbar_norms'], extensions=['png']) |
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.
Image test looks reasonable (no need to overthink into avoiding them either -- unless someone has a better idea...); perhaps you can use style="default"? (ticks inwards look weird to me now :-), and then you don't need to specify viridis below explicitly)
Thanks for looking at this so quickly, @dstansby! To get around the images I thought you might be able to look at coordinates from a transform within the axes and compare them to the norm somehow (make a 0-1 axis and then |
test failure seems real |
Yep, they are. Not sure I have time in the next couple of days to fix, so if anyone else fancies investigating, fixing and pushing to my branch before then feel free. |
I pushed to your branch but ci doesn’t seem to like it. Can your force push the change? |
dd435a1
to
3b
8000
32e16
Compare
I figured out the force push |
This is a situation where the Norm not having an accompanying scale is what is causing all the problems. The current fix is fine, but it basically special cases For My suggestion would be for a bit of re-architecting here, and make each norm have a scale property that colorbar could consult rather than us having to guess the scale. That would also trigger folks who write Norms to create a corresponding scale so the colorbar can work with the a real y scale rather than a faked one. |
I'm OK with this as is for a temporary fix - it passes CI except for a Pandas deprecation warning, which isn't this PR. |
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.
Actually I'm going to block this for now. The colorbars are still not correct, which you can easily see by changing the Twoslope norm to something that has two slopes....
I also don't think the symmetric log plot is correct above - the linear scale is supposed to be one decade of the logarithmic scale, and its clearly not. It would be interesting to see if it ever was, but I doubt it. This will take some sitting down to figure out and may take me a few weeks as I have other deadlines. |
The SymLog code has been broken forever I think, and I have been working on it on and off recently. Hopefully I'll have something to show for it soon, thanks for opening the issue. |
Given that @anntzer has a factory for turning scales into norms, that would really make our lives a lot easier as we'd only need to make sure the Transform is correct, rather than rewriting one in the Norm. I think if we have a colornorm it should have a corresponding scale for the colorbar, so enforcing some regularity would be good. |
Not all of the warnings from pandas report that they are from pandas. Closes matplotlib#16295 (again)
ImageGrid is so little-maintained that I don't think we should have duplicate argument lists in the tutorial and the docstring, given that they *will* go out of sync (for more commonly used APIs, sure, we can maintain the duplication, but here it's not worth it). Add to the docstring the relevant parts of the tutorial, and make the tutorial point to the API docs.
Sorry I botched this up. I'll push this as a new PR with my apologies. |
Fixes #16280, and also fixes colorbars with a few other norms.
I have also added an image test. I can't think of a way to test this without an image, but suggestions welcome. Before making this change, the test image with different norms looks like this on current master:
Whereas after the fix it is:
cc @jklymak