-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Conversation
Arrays of non-uint8 types were assumed to be in 0..1 range. When this was not true and integer values were used, only the low-order bits were used, resulting in a mangled image. Now, an explicit exception is raised. Closes issue matplotlib#2499
❤️ pep8 |
MNT: improve image array argument checking in to_rgba. Closes #2499.
if not bytes and xx.dtype == np.uint8: | ||
xx = xx.astype(float) / 255 | ||
if xx.dtype.kind == 'f': | ||
if xx.max() > 1 or xx.min() < 0: |
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 to imshow
with interpolation 'none', will it be run through to_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.
Is this going to be backported? While it's milestoned 2.1, the original issue is milestoned 2.0. |
I am 50/50 on backporting. |
And we should wait for @mdboom 's follow on commit in either case. |
No description provided.