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

FIX: handle nans in RGBA input with ScalarMappables #27848

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Mar 3, 2024

PR summary

Fixes #27301 by setting alpha to zero at points where any value is nan. This is consistent with the handling added for masked arrays at #26096.

Note this PR competes with #27303 which goes the other way and raises when nans are in the array.

PR checklist

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

I'm sympathetic to the view that it is easier to push in a "nan" and expect nothing to be plotted rather than to be forced into using masked arrays by raising a ValueError if a nan appears. I can see both sides here though. This seems like a small amount of code, so nothing crazy complex to support this use-case.

lib/matplotlib/cm.py Outdated Show resolved Hide resolved
# If any of R, G, B, or A are masked for an entry, we set alpha to 0
xx[np.any(np.isnan(x), axis=2), 3] = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to move this up into only the floating point portion so we aren't checking for nan's on non-float data.

else:
raise ValueError("Third dimension must be 3 or 4")
if xx.dtype.kind == 'f':
if norm and (xx.max() > 1 or xx.min() < 0):
raise ValueError("Floating point image RGB values "
"must be in the 0..1 range.")
# If any of R, G, B, or A is nan, set to 0
xx[np.any(np.isnan(x), axis=2), :] = 0
Copy link
Member Author

Choose a reason for hiding this comment

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

The casting for the bytes=True case will error if there are nans in the array so we need to replace the nan with an arbitrary float. Setting the whole pixel to 0 seems the simplest way to do that.

Copy link
Member

Choose a reason for hiding this comment

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

I think this change is too late; if xx = np.array([10, np.nan, 0.2]), then the above check should fail, but it doesn't, because xx.max() == np.nan, and np.nan > 1 is False.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I just move this above the range check, then we replace the pixel with zeros which will make the range check pass. So there will be no difference. Do we want to do something more complicated than set the whole pixel to zero?

Copy link
Member

Choose a reason for hiding this comment

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

That depends on if we want to ignore all of the pixel that is NaN, regardless of value or not. We can replace with np.nanmax/np.nanmin in that case. If not, then we need to do something more complicated.

But my point is more that the check is broken even if the NaN and the invalid value are not in the same pixel.

Copy link
Member Author

Choose a reason for hiding this comment

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

But my point is more that the check is broken even if the NaN and the invalid value are not in the same pixel.

Oh yes, I get it now.

@rcomer rcomer closed this Mar 4, 2024
@rcomer rcomer reopened this Mar 4, 2024
@tacaswell
Copy link
Member

I think the question is in the case of RGBA images do we want:

  • np.nan to act like np.inf and fail the whole things
  • np.nan to act as a missing value like we do in most other places in the library

On net I think I come down in favor of this PR over mine.

lib/matplotlib/cm.py Outdated Show resolved Hide resolved
@timhoffm
Copy link
Member

timhoffm commented Mar 7, 2024

Doc build fails because of warnings from #27658

@dstansby
Copy link
Member

dstansby commented Mar 7, 2024

I think a rebase on to main should fix the doc build.

@rcomer
Copy link
Member Author

rcomer commented Mar 7, 2024

Update following discussion on call. The walrus is for @tacaswell :-)

@timhoffm timhoffm merged commit c1a55a4 into matplotlib:main Mar 7, 2024
41 of 42 checks passed
@rcomer rcomer mentioned this pull request Mar 8, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: imshow allows RGB(A) images with np.nan values to pass
6 participants