NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

Fix logic error in ScalarMappable.to_rgba#11002

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

Jump to bottom

Merged

efiring merged 1 commit intomatplotlib:masterfromtimhoffm:fix-to_rgba-logic-error

Apr 9, 2018

Merged

Fix logic error in ScalarMappable.to_rgba#11002

efiring merged 1 commit intomatplotlib:masterfromtimhoffm:fix-to_rgba-logic-error

Apr 9, 2018

Conversation

Copy link

Member

timhoffm commentedApr 8, 2018

PR Summary

This fixes the logic error described in#10890.

fix logic error in ScalarMappable.to_rgba

0b6fff1

efiring mentioned this pull request

Apr 8, 2018

Correct logical expression in to_rgba().#11003

Closed

6 tasks

efiring approved these changes

Apr 8, 2018

View reviewed changes

tacaswell added this to thev2.2.3 milestone

Apr 9, 2018

Copy link

Member

tacaswell commentedApr 9, 2018

I think this is a candidate for backport because it fixes a un-workaroundable bug in 2.2.2. I am not 100% on that, if someone else agrees merge away!

tacaswell approved these changes

Apr 9, 2018

View reviewed changes

Copy link

Member

jklymak commentedApr 9, 2018•
edited
Loading

I actually am 98.5% sure itsnot a followed code path because of the new clipping for imshow (#10220). 0% codecov isn't very good ;-)

(i.e. max(x)>1 is alwaysFalse)

Copy link

Member

tacaswell commentedApr 9, 2018

This is (for better or worse) a public function, I think the clipping happens when we set the data so using the 'standard' path this won't be hit, but usingim.to_rgba(my_data) can still hit it.

Copy link

Member

jklymak commentedApr 9, 2018

Ah, fair enough.

OTOH, should it get a test if its public, including one to test if this error is properly called? But happy to see it merged if thats gilding the lily...