Skip to content
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

BUG: fix crash in image normalization for RGBA image arrays #4094

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Aug 22, 2022

PR Summary

Fix #4093

the bleeding edge CI job is running on my fork at https://github.com/neutrinoceros/yt/actions/runs/2911689231

@neutrinoceros neutrinoceros added bug release critical Highest priority (in a milestone) labels Aug 22, 2022
@neutrinoceros neutrinoceros added this to the 4.1.0 milestone Aug 22, 2022
if data.ndim == 3:
assert data.shape[-1] == 4
# this is an RGBA array, only linear normalization makes sense here
norm_type = Normalize
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's RGBA then I'm not sure I understand in what cases we'd want normalization. If it's not already scaled 0..1 or 0..255, then we need to apply that manually, and I don't think we want colorbars for that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, this is just the simplest patch to restore previous behaviour. Before merging #3849 we used to pass a Normalize object in _init_image, and MPL didn't complain, because it's what it uses by default.
A different approach would be to patch this in _init_image, though I have a strong preference for leaving this responsability entirely to the NormHandler class because it's its sole purpose. Does it make sense ? Maybe I can add a little more details in the comment here ?

@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@neutrinoceros neutrinoceros force-pushed the hotfix_rgba_image_norm branch from 617ab0b to 7ec78b6 Compare August 23, 2022 12:54
@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@neutrinoceros neutrinoceros force-pushed the hotfix_rgba_image_norm branch from 7ec78b6 to db68135 Compare August 23, 2022 15:43
@neutrinoceros neutrinoceros marked this pull request as ready for review August 23, 2022 16:28
@neutrinoceros
Copy link
Member Author

@matthewturk if you get the chance, please review this again

@neutrinoceros neutrinoceros added the blocker Highest priority level label Aug 24, 2022
@matthewturk matthewturk enabled auto-merge August 24, 2022 18:41
@matthewturk matthewturk merged commit 6f2a542 into yt-project:main Aug 24, 2022
@neutrinoceros neutrinoceros deleted the hotfix_rgba_image_norm branch August 24, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Highest priority level bug release critical Highest priority (in a milestone)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: regression with future MPL
2 participants