Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
dstansby wants to merge10 commits intomatplotlib:masterfromdstansby:fix-cbars

Conversation

dstansby
Copy link
Member

@dstansbydstansby commentedJan 21, 2020
edited
Loading

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

@dstansbydstansby added Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: color/colorbar labelsJan 21, 2020
@dstansbydstansby added this to thev3.2.0 milestoneJan 21, 2020
@dstansbydstansby changed the titleFix cbarsFix cbars for different normsJan 21, 2020
@jklymak
Copy link
Member

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

@anntzeranntzerJan 21, 2020
edited
Loading

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 tooverthink into avoiding them either -- unless someone has a better idea...); perhaps you can use? (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 thentr = 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

test failure seems real

@dstansby
Copy link
MemberAuthor

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

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

@jklymakjklymak mentioned this pull requestJan 30, 2020
6 tasks
@jklymak
Copy link
Member

I figured out the force push

@jklymak
Copy link
Member

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

ForSymLogNorm andLogitNorm there are scales, but I don't see one forPowerNorm.

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

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

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

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
MemberAuthor

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

Given that@anntzer has a factory for turning scales into norms, that would really make our lives alot 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.

tacaswelland others added2 commitsFebruary 2, 2020 19:12
Not all of the warnings from pandas report that they are from pandas.Closesmatplotlib#16295 (again)
maxhumberand others added3 commitsFebruary 2, 2020 19:12
ImageGrid is so little-maintained that I don't think we should haveduplicate argument lists in the tutorial and the docstring, given thatthey *will* go out of sync (for more commonly used APIs, sure, we canmaintain the duplication, but here it's not worth it).  Add to thedocstring the relevant parts of the tutorial, and make the tutorialpoint to the API docs.
@jklymak
Copy link
Member

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@jklymakjklymakjklymak requested changes

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.status: duplicatetopic: color/colorbar
Projects
None yet
Milestone
v3.2.0
Development

Successfully merging this pull request may close these issues.

SymLogNorm colorbar incorrect on master
7 participants
@dstansby@jklymak@greglucas@anntzer@QuLogic@tacaswell@maxhumber

[8]ページ先頭

©2009-2025 Movatter.jp