Don't convert vmin, vmax to floats.

[anntzer]

They may be float128's in which case precision would be lost; this can
result in Normalize returning values (barely) outside of [0, 1].

(The cast to float was introduced in 28e1d2, referring to bug 2997687
on SF; it may be worth checking what it was about.)

See #6698.

[efiring]

I suspect it would make more sense to just convert the float128 input to float64 at an early stage. The conversion will occur sooner or later anyway; we have no mechanism for maintaining float128 throughout the pipeline, and that level of precision makes no sense for plotting anyway.

[WeatherGod]

Eventually, everything needs to become float-like. The purpose of casting
to float was most likely to avoid integer division. Unfortunately, the
issue tracker is no longer up on sourceforge, so it is going to be hard to
find out what that particular issue was.

On Wed, Jul 6, 2016 at 3:34 PM, Antony Lee notifications@github.com wrote:

They may be float128's in which case precision would be lost; this can
result in Normalize returning values (barely) outside of [0, 1].

(The cast to float was introduced in 28e1d2, referring to bug 2997687
on SF; it may be worth checking what it was about.)

See #6698 #6698.

You can view, comment on, or merge this pull request online at:

#6700
Commit Summary

• Don't convert vmin, vmax to floats.

File Changes

• M lib/matplotlib/colors.py
  https://github.com/matplotlib/matplotlib/pull/6700/files#diff-0 (5)
• M lib/matplotlib/tests/test_colors.py
  https://github.com/matplotlib/matplotlib/pull/6700/files#diff-1 (5)

Patch Links:

• https://github.com/matplotlib/matplotlib/pull/6700.patch
• https://github.com/matplotlib/matplotlib/pull/6700.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6700, or mute the thread
https://github.com/notifications/unsubscribe/AARy-KbEYAVFrGM6K78a6IvYiNORYcJIks5qTANWgaJpZM4JGbL8
.

[anntzer]

See also #6677 for @efiring's suggestion (which I agree with -- similar bugs with handling float128's probably occur at a bunch of other places). However I'd rather keep this simple solution for now and have a separate discussion for making the switch to float64 (or even float32, which is certainly enough for plotting purposes) everywhere at the same time.

PS: The failure on Travis seems spurious.

[tacaswell]

There is no float128 on windows?

[anntzer]

Apparently not... https://mail.scipy.org/pipermail/scipy-dev/2008-March/008562.html
A quick test tells me that longdouble is float64 on Windows. Still, that seems good enough for the tests. Updated.

[tacaswell]

It looks like the single value branch of the process_value method needs the same sort of logic as the array branch. I think it will currently cast a single float128 to a possibly shorter float leading to the same issue.

[anntzer]

Actually both branches probably need it, right?
I think we should cast result to np.promote_types(result.dtype, np.float32) (i.e. at least float32 but not smaller than the actual size).

[tacaswell]

The array branch only casts to it if it is not a float type, otherwise we just copy.

[anntzer]

I see. I guess the whole process_value method could be simplified to

def process_value(value):
    is_scalar = not cbook.iterable(value)
    if is_scalar:
        value = [value]
    dtype = np.min_scalar_type(value)
    dtype = (np.float32 if dtype.itemsize <= 2
             else np.promote_types(dtype, float))
    result = np.ma.array(value, dtype=dtype, copy=True)
    return result, is_scalar

right?

[tacaswell]

Yes, I think so.

[anntzer]

done.

[tacaswell]

👍 LGTM pending appveyor

[anntzer]

Appveyor failure seems spurious?
Also, note that the parent issue (#6698) is milestoned to 2.0 (and this bug was revealed by #6382, which is also in 2.0).

[jenshnielsen]

Regarding float128 it's not actually 128 bytes but a long double which is padded to 128 bits http://docs.scipy.org/doc/numpy-dev/user/basics.types.html

On Linux and OSX long doubles are normally 80 bits but on windows (MSVC) they are equivalent to doubles. They may have more precision on other less common platforms.

[tacaswell]

backported to v2.x as c7d7d19

[anntzer]

@efiring @WeatherGod @tacaswell
Following the discussion in #6825 I will probably need to revert that PR for a more comprehensive solution instead, which is unlikely to be ready by 2.0 (any timeline btw?). However, we still need a fix for #6698 for 2.0. I think we can probably get away with giving a bit of tolerance to the 0-1 range (i.e. clip values between -1e-8 and 0 to 0 and values between 1 and 1+1e-8 to 1). Thoughts?