8000 Fix cbars for different norms by dstansby · Pull Request #16286 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

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

Closed
wants to merge 10 commits into from
Closed

Conversation

dstansby
Copy link
Member
@dstansby dstansby commented Jan 21, 2020

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:

colorbar_norms_before

Whereas after the fix it is:

colorbar_norms

cc @jklymak

@dstansby dstansby added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: color/colorbar labels Jan 21, 2020
@dstansby dstansby added this to the v3.2.0 milestone Jan 21, 2020
@dstansby dstansby requested a review from jklymak January 21, 2020 19:43
@dstansby dstansby changed the title Fix cbars Fix cbars for different norms Jan 21, 2020
@jklymak
Copy link
Member
jklymak commented Jan 21, 2020

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'])
Copy link
Contributor
@anntzer anntzer Jan 21, 2020

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)

@greglucas
Copy link
Contributor

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 tr = cbar.ax.transData + fig.transFigure.inverted() or something similar to back out where the tick(x) is versus the norm(x), but I think the issue is that these norms that fail don't have an inverse which defeats that route. Maybe you can think of a better way of doing that though to avoid the image test if you're inclined. But, I also think the image fine.

@anntzer
Copy link
Contributor
anntzer commented Jan 24, 2020

test failure seems real

@dstansby
Copy link
Member Author

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.

@jklymak
Copy link
Member
jklymak commented Jan 30, 2020

I pushed to your branch but ci doesn’t seem to like it. Can your force push the change?

@jklymak jklymak mentioned this pull request Jan 30, 2020
6 tasks
@jklymak
Copy link
Member
jklymak commented Jan 30, 2020

I figured out the force push

@jklymak
Copy link
Member
jklymak commented Jan 30, 2020

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 LogNorm and Normalize, and everything else is handled on a fake linear scale.

For SymLogNorm and LogitNorm there are scales, but I don't see one for PowerNorm.

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.

@jklymak
Copy link
Member
jklymak commented Jan 30, 2020

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.

Copy link
Member
@jklymak jklymak left a 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....

@jklymak
Copy link
Member
jklymak commented Jan 30, 2020

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.

@dstansby
Copy link
Member Author

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.

@jklymak
Copy link
Member
jklymak commented Jan 31, 2020

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.

tacaswell and others added 2 commits February 2, 2020 19:12
Not all of the warnings from pandas report that they are from pandas.

Closes matplotlib#16295 (again)
maxhumber and others added 3 commits February 2, 2020 19:12
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.
@jklymak
Copy link
Member
jklymak commented Feb 3, 2020

Sorry I botched this up. I'll push this as a new PR with my apologies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: duplicate topic: color/colorbar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SymLogNorm colorbar incorrect on master
7 participants
0