Fix crash when imshow encounters longdouble data #10768

br-Zhang · 2018-03-13T02:24:45Z

PR Summary

This PR solves #10342. Support for longdouble was added in #8447, however attempting to draw anything causes a crash. This PR adds a cast to float64 when imshow encounters longdouble data.

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

anntzer · 2018-03-13T04:27:13Z

So if I understand correctly, #8447 is essentially pointless? #8447 added downcasting of float128 to float64 at normalization time, but we always run interpolation first and that step already needs to work on float64?

br-Zhang · 2018-03-13T12:17:28Z

I believe that is correct.

jklymak · 2018-03-13T16:15:02Z

If we just down-cast to float64, that's not really supporting float128, its just not crashing. I think that float128 should error, maybe w/ a better error, and tell users they need to cast themselves, given that opinions vary wildly as to how down-casting should happen. ping @efiring who had an opinion about all this when I added float128 before.

efiring · 2018-03-13T18:10:21Z

Maybe downcast with a warning. Effectively, as a plotting library, we are always downcasting drastically, since plots have very limited combinations of dynamic range and resolution. I presume the use of float128 is very rare, and I don't see any way that "supporting" it in matplotlib is going to help anyone do anything useful. Use of float128 should be very rare because it is wildly platform-dependent, and doesn't even provide the advertised 128 bits. It is not included in the numpy table of supported types:
https://docs.scipy.org/doc/numpy/user/basics.types.html
The situation is explained at the bottom of that section:
https://docs.scipy.org/doc/numpy/user/basics.types.html#extended-precision
Note that there is also a float96 which should be handled in the same (minimal) way as float128.

jklymak · 2018-03-13T18:16:11Z

I'd be fine with downcast-with-warning... For this PR, you'd want to add the warning, and an assert that its emited in the test...

anntzer · 2018-03-13T18:26:32Z

Probably also worth (effectively) reverting #8447 then, given that by the time we reach colormapping, the data is certainly not float128 anymore? (dunno)

br-Zhang · 2018-03-13T20:42:32Z

I've updated the pull request to add a warning before casting, remove the redundant check for casting in colors.py, and added an assert in the test to ensure that the warning is emitted.

jklymak · 2018-03-14T00:14:45Z

lib/matplotlib/image.py

@@ -361,6 +362,12 @@ def _make_image(self, A, in_bbox, out_bbox, clip_bbox, magnification=1.0,
                    a_min, a_max = np.int32(0), np.int32(1)
                if inp_dtype.kind == 'f':
                    scaled_dtype = A.dtype
+                    # Cast to float64
+                    if A.dtype == np.longdouble:


OK, still not sure what to do about np.float128 and np.float96. Should these be trapped here as well?

Based on the platform version, np.float128 or np.float96 may or may not exist so adding traps for them might not work. Instead, I think casting anything that isn't float64, float32 or float16 to float64 would be a better idea.

Really, they aren't even defined? Yuck....

Yes. np.longdouble is platform dependent also. That's why I most likely have to remove the assert for warnings from the test case since the CI runs on Windows which has np.longdouble == 'float64' == 'float'. This causes either excessive warnings which will break all sorts of CI tests (if I choose to show warnings for each cast) or no warnings at all (if I only show warnings on casts from a larger size to smaller).

jklymak · 2018-03-14T05:42:05Z

Given the issues with longdouble float128 etc maybe the right thing to do is let this error?

br-Zhang · 2018-03-14T17:19:31Z

The problem with longdouble is mostly with testing. The changes in this PR makes a downcast to float64 whenever it encounters longdouble, float96 or float128, which is working fine. The issue is that, on Windows, longdouble == float64 so a trap/error for longdouble would cause an error on float/float64 too. Adding a check for float64 solves this but means that warnings will only be emitted on platforms where longdouble == float128/float96, which makes it hard to write a test that asserts for a warning.

QuLogic · 2018-03-14T21:37:08Z

lib/matplotlib/image.py

@@ -361,6 +362,13 @@ def _make_image(self, A, in_bbox, out_bbox, clip_bbox, magnification=1.0,
                    a_min, a_max = np.int32(0), np.int32(1)
                if inp_dtype.kind == 'f':
                    scaled_dtype = A.dtype
+                    # Cast to float64
+                    if not (A.dtype == np.float32 or A.dtype == np.float16):


if A.dtype not in (np.float32, np.float16):

QuLogic · 2018-03-14T21:37:27Z

lib/matplotlib/image.py

@@ -361,6 +362,13 @@ def _make_image(self, A, in_bbox, out_bbox, clip_bbox, magnification=1.0,
                    a_min, a_max = np.int32(0), np.int32(1)
                if inp_dtype.kind == 'f':
                    scaled_dtype = A.dtype
+                    # Cast to float64
+                    if not (A.dtype == np.float32 or A.dtype == np.float16):
+                        if (A.dtype != np.float64):


Remove unnecessary parentheses.

jklymak · 2018-03-14T21:56:29Z

lib/matplotlib/image.py

@@ -361,6 +362,13 @@ def _make_image(self, A, in_bbox, out_bbox, clip_bbox, magnification=1.0,
                    a_min, a_max = np.int32(0), np.int32(1)
                if inp_dtype.kind == 'f':
                    scaled_dtype = A.dtype
+                    # Cast to float64
+                    if A.dtype not in (np.float32, np.float16):
+                        if A.dtype != np.float64:


might as well just make this one if-statement....

There are things happening after this one but not in it...

if A.dtype not in (np.float32, np.float16, np.float64): warnings.warn() scaled_dtype = np.float64

should do it, right?

I actually did this in commit a453ba4. This led to problems in some Windows environments and failures in CI. It seems like scaled_dtype needs to be set to np.float64 instead of np.longdouble even if np.longdouble == np.float64.

not in returns something different than !=? I'm not following the logic here at all.

It's very platform specific. Running on Win-64 gives:

>>> numpy.longdouble in (numpy.float16, numpy.float64) False

But numpy.longdouble == numpy.float64 returns true?

Very weird.

>>> numpy.longdouble == numpy.float64 False

Again, the problem doesn't have anything to do with the check here; it's the use of scaled_dtype later that causes a problem. If you flatten the condition, then scaled_dtype=np.longdouble is not replaced by np.float64 (on Windows only!) It's then crashing in C code later (which is arguably a NumPy inconsistency) which doesn't understand np.longdouble.

OK; I get it now. everything not np.float16/32 has to be explicitly set to np.float64, even if python thinks np.longdouble is equivalent to np.float64, because the C code doesn't think its equivalent.

Sorry to be so dense, but I do think its pretty bad that the C code ends up incompatible in this way.

QuLogic

This should be squashed.

QuLogic · 2018-03-15T01:12:26Z

lib/matplotlib/image.py

@@ -10,6 +10,7 @@
 from io import BytesIO

 from math import ceil
+import warnings


This belongs after logging.

br-Zhang · 2018-03-15T03:33:57Z

Squashed all the previous commits into one commit.

jklymak · 2018-03-15T03:53:54Z

I'll merge tomorrow if no one does so earlier. Just want to leave it up if anyone else wants to chime in. But I think I understand what is going on now and this fix seems fine.

br-Zhang changed the title ~~Fix crash when imshow encouters longdouble data~~ Fix crash when imshow encounters longdouble data Mar 13, 2018

QuLogic requested a review from anntzer March 13, 2018 03:43

jklymak reviewed Mar 14, 2018

View reviewed changes

anntzer removed their request for review March 14, 2018 00:17

QuLogic reviewed Mar 14, 2018

View reviewed changes

jklymak reviewed Mar 14, 2018

View reviewed changes

QuLogic approved these changes Mar 15, 2018

View reviewed changes

Fix crash when imshow encounters longdouble.

809353c

br-Zhang force-pushed the bugfix-for-issue-10342 branch from cc74d0b to 809353c Compare March 15, 2018 01:44

jklymak approved these changes Mar 15, 2018

View reviewed changes

jklymak added this to the v3.0 milestone Mar 15, 2018

jklymak merged commit f49c7b3 into matplotlib:master Mar 15, 2018

dstansby mentioned this pull request Apr 8, 2018

imshow longdouble not truly supported #10342

Closed

Uh oh!

Fix crash when imshow encounters longdouble data #10768

Fix crash when imshow encounters longdouble data #10768

Uh oh!

Conversation

br-Zhang commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

anntzer commented Mar 13, 2018

Uh oh!

br-Zhang commented Mar 13, 2018

Uh oh!

jklymak commented Mar 13, 2018

Uh oh!

efiring commented Mar 13, 2018

Uh oh!

jklymak commented Mar 13, 2018

Uh oh!

anntzer commented Mar 13, 2018

Uh oh!

br-Zhang commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

br-Zhang Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

br-Zhang Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Mar 14, 2018

Uh oh!

br-Zhang commented Mar 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

br-Zhang Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

br-Zhang Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

br-Zhang commented Mar 13, 2018 •

edited

Loading

br-Zhang commented Mar 13, 2018 •

edited

Loading

br-Zhang Mar 14, 2018 •

edited

Loading

br-Zhang Mar 14, 2018 •

edited

Loading

jklymak Mar 14, 2018 •

edited

Loading

br-Zhang Mar 14, 2018 •

edited

Loading

br-Zhang Mar 14, 2018 •

edited

Loading