Skip to content

Bug causes to_rgba to fail inside cm.py #10890

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

Closed
Eagleames opened this issue Mar 27, 2018 · 19 comments
Closed

Bug causes to_rgba to fail inside cm.py #10890

Eagleames opened this issue Mar 27, 2018 · 19 comments
Milestone

Comments

@Eagleames
Copy link

Eagleames commented Mar 27, 2018

I was running a test with some new images and plotting, code called cm.to_rgba() to convert the files for keras processing.

Matplotlib 2.1.2 (but verified in the current repository)

It 'works' however it throws an error, by mistake due to this error in code:

                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 xx.min() <0 then this code piece triggers.

Should be:

                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.")

Sorry.. not familiar enough with making pull requests and changes myself to do this!

@jklymak
Copy link
Member

jklymak commented Mar 27, 2018

Can you provide a minimal example that triggers this? Thanks!

@Eagleames
Copy link
Author

if the input image has been normalized to zero (-127 to +127 range) and is a float it will trigger this section.
However if you called norm = False, this section will still trigger erroneously.
Error looks like this: (note that norm = False, and in this case xx.min() was -120)


  File "e:\anaconda3\lib\site-packages\matplotlib\image.py", line 484, in _make_image
    output = self.to_rgba(output, bytes=True, norm=False)

  File "e:\anaconda3\lib\site-packages\matplotlib\cm.py", line 259, in to_rgba
    raise ValueError("Floating point image RGB values "

ValueError: Floating point image RGB values must be in the 0..1 range.

@jklymak
Copy link
Member

jklymak commented Mar 27, 2018

Can you provide a minimal self contained example written in python that doesn’t use an external file that triggers this? Thanks.

@Eagleames
Copy link
Author

Eagleames commented Mar 27, 2018

Wow. ok... that was an adventure. Hope this is meaningful...
I extracted a piece of the post-treated (by Keras VGG16 preprocessing) image and then saved that to an array. When you plot it... you get this error!

import matplotlib.pyplot as plt
import numpy as np

test = np.array([[[ 2.4265968e+01, -7.1251709e+01,  1.1596016e+01],
        [ 4.1356895e+01, -6.1591396e+01,  1.8787498e+01],
        [ 6.0224663e+01, -5.3484486e+01,  2.7164375e+01],
        [ 6.2998756e+01, -6.2542957e+01,  2.8223625e+01],
        [ 5.0635292e+01, -8.5200035e+01,  2.2165154e+01]],

       [[ 3.0379570e+01, -5.6437828e+01,  1.2119255e+01],
        [ 4.8944832e+01, -4.6965721e+01,  2.5350716e+01],
        [ 6.3876762e+01, -4.4302704e+01,  3.1537758e+01],
        [ 5.7498558e+01, -6.3408409e+01,  2.1198906e+01],
        [ 4.3169197e+01, -8.9243187e+01,  1.1302132e+01]],

       [[ 3.0193733e+01, -5.1721474e+01,  5.0541916e+00],
        [ 4.6437859e+01, -4.7197594e+01,  2.3918770e+01],
        [ 5.5936256e+01, -4.9692642e+01,  2.7207161e+01],
        [ 4.3793571e+01, -6.9284180e+01,  9.7025375e+00],
        [ 3.2357903e+01, -8.4746979e+01,  1.1040497e-01]],

       [[ 2.8536415e+01, -6.0043530e+01, -6.5765381e-03],
        [ 4.0998665e+01, -5.7634468e+01,  2.0513100e+01],
        [ 4.3069878e+01, -6.6510132e+01,  1.6839577e+01],
        [ 3.2806328e+01, -7.4617233e+01,  2.3700790e+00],
        [ 3.2372643e+01, -6.7550941e+01,  3.2968445e+00]],

       [[ 3.3632256e+01, -7.0232697e+01,  3.0147018e+00],
        [ 4.4523140e+01, -7.1141457e+01,  2.0268974e+01],
        [ 3.8679253e+01, -8.2500259e+01,  1.0053246e+01],
        [ 3.2569786e+01, -7.6080200e+01,  3.6950302e+00],
        [ 3.4381584e+01, -5.4521236e+01,  7.9723282e+00]]], dtype=np.float32)

plt.imshow(test)
plt.show()

@jklymak
Copy link
Member

jklymak commented Mar 27, 2018

This was fixed in 2.2 so I think it works in master. I get

% python testimshow.py
Clipping input data to the valid range for imshow with RGB data ([0..1] for floats or [0..255] for integers).

figure_1-3

@Eagleames
Copy link
Author

Ah, so they fixed it by forcing you to normalize... when the real problem is that someone messed up on a logic statement...
if NORMALIZED and (BIGGER THAN ONE or BIGGER THAN ONE) <= This makes sense.
Currently:
if NORMALIZED and BIGGER THAN ONE or SMALLER THAN ZERO <= This will always trigger the error flag EVEN when you do NOT have normalized data.

If you add the parentheses correctly, the function will work on non-normalized floats!

I think it was fixed and reduced functionality incorrectly.

@Eagleames
Copy link
Author

Eagleames commented Mar 27, 2018

Note that the old code would catch the errors without the clipping requirement.... (with parentheses)

@jklymak
Copy link
Member

jklymak commented Mar 27, 2018

For 2.2 we enforce the range 0-1 by clipping, regardless of the value of norm (#10220) . If you change the bracket this same clipping occurs.

In 2.1 this same code threw an error. I agree that if you change the brackets it does not throw an error, and returns a plot. I'm not sure how the plot values get turned into valid RGBA values. I'd have to look more.

But all the documentation states that if you pass floats, they should be normalized between 0 and 1. I don't think the current behaviour is incorrect to enforce that. However, I'm sure we can discuss, particularly if this is a regression.

@Eagleames
Copy link
Author

I suppose that if the documentation says that the input should be normalized... it is the 'correct' place to go from, however it does make it so that this error check is meaningless as neither the normalize or the value checks can ever fail.

I can tell you that if you just put the brackets in (2.1.2) the result you get is a graph of the data normalized across the full color range... unfortunately I didn't take a screenshot and just updated to 2.2.2.

My feeling is that the normalization requirement should only be there if it is truly needed, and since it is not... It is better to effectively do the normalization in the color space rather than to corrupt the data by clipping.

If the normalized input was a hard requirement, I would prefer it throw an error if I passed it un-normalized data rather than show me a false image (with a screen flag I might not catch).

This can be the kind of problem you chase your tail around for hours as a student... not realizing how 'severe' the clipping was in your image. (note that up to 100x range is lost in the clipping for our example above)

@jklymak
Copy link
Member

jklymak commented Mar 27, 2018

There is a big warning that tells you your data was clipped and what to do about it.

My personal opinion is that you can't just normalize all data because sometimes a data set with a range between 0.2 and 0.4 is meant to be comapred to a data range between 0 and 1. So do you only auto normalize if you are outside 0-1? At some point the user has to decide, so I'm pretty happy with a hard clip and warn strategy.

@jklymak jklymak added this to the v3.0 milestone Mar 27, 2018
@Eagleames
Copy link
Author

I get your point, it is all about desired affect vs. hidden effect.

If you want to compare things that are 0.2 to 0.4 and 0.4 to 0.6 will it compare them 'correctly'?

Yes - according to the documentation. So I have to agree that is the right way to go due to the fact that the 'auto normalize' that I was seeing could hide this effect on you (and gives you something outside of the docs as well.

But.... the original parentheses... it is an error :-)

@Eagleames
Copy link
Author

I am not sure if I was supposed to close that or not... :-/

@jklymak
Copy link
Member

jklymak commented Mar 27, 2018

Up to you! If you think there is more dicsussion to be had, I'm fine for it to be re-opened. I'm just a junior member around here, and not always right by a long stretch, so others may want to chime in.

@Eagleames
Copy link
Author

Eagleames commented Mar 27, 2018

Not sure how to re-open. Someone should fix those parentheses at least for OCD sake X-D

Ok... I'm learning...

@Eagleames Eagleames reopened this Mar 27, 2018
@efiring
Copy link
Member

efiring commented Apr 8, 2018

The statement is correct as-is because "or" has higher precedence than "and":
https://docs.python.org/3/reference/expressions.html#operator-precedence
so adding the parentheses would aid readability but would not change the behavior. I don't think that the small readability improvement here is worth a new PR.

@efiring efiring closed this as completed Apr 8, 2018
@Eagleames
Copy link
Author

That is very interesting as adding the parentheses and running locally caused a change in behavior for me.

@QuLogic
Copy link
Member

QuLogic commented Apr 8, 2018

@efiring you have read the table backwards; the top is lowest precedence, so or is lower than and.

@QuLogic QuLogic reopened this Apr 8, 2018
@efiring
Copy link
Member

efiring commented Apr 8, 2018

Oops! Sorry.

efiring added a commit to efiring/matplotlib that referenced this issue Apr 8, 2018
@tacaswell tacaswell modified the milestones: v3.0, v2.2.3 Apr 9, 2018
@efiring
Copy link
Member

efiring commented Jul 2, 2018

The original error was fixed, so I don't see why this is still open. If there is still a problem, since it is not the original one, please open a new issue for it.

@efiring efiring closed this as completed Jul 2, 2018
@matplotlib matplotlib deleted a comment from biswajitcsecu Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants