-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
MNT: improve image array argument checking in to_rgba. Closes #2499. #6122
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Kind of an expensive check for something that's on the mainline for displaying images. What happens if we remove this check?
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.
How does this interact with your image refactor? I think that if this check is executed only on external input of 3-D RGBA arrays, then it is OK; that is really its purpose here. But if it is executed on internally-generated arrays in the normal course of events, then it needs to be bypassed. In fact, if that is happening, perhaps it is an entire call to the
to_rgba
method that needs to be bypassed, and the check can stay in place.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.
That's a good point. I do think it will only get run on externally-provided images -- but that does include images loaded using imread. I still wonder if it's too expensive even in that case though.
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.
At least post-image-refactor these images are at "screen resolution" and not at data resolution which could be larger.
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.
sorry for being trigger happy on this.
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.
Having poked at it, for image I think it's impossible for this case to even happen -- the data is normalized (in any event) before it gets here. (However, I suspect that's not the case for other places to_rgba is used, like scatter).
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.
I think uses like scatter are irrelevant because the checking in question is invoked only for MxNx3 and MxNx4 inputs, which are assumed to be RGB or RGBA images.
As for "impossible":
to_rgba
is not private, so it is entirely possible for a user to generate a 3-D float array that would fail the check. If such an array is fed toimshow
with interpolation 'none', will it be run throughto_rgba
? Certainly it will not be normalized.The options are to remove the check on the grounds that it is usually unnecessary (we have managed without it for a long time), to leave it in and accept the performance penalty, or to find a way to distinguish between cases where it might be useful, if such cases exist, and cases where we are sure it is pointless. The last probably would be the best if it can be accomplished with minimal added complexity.
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.
As a middle road, I think we can safely assume if
norm=False
is passed in that it's already normalized and we don't need to do this check. That will catch most of the cases where the check would be redundant.