homepage

math.nan fails on some Decimal NANs. For example, while this works:
 >>> import math
 >>> from decimal import Decimal
 >>> math.isnan(Decimal('nan'))
 True
These both fail with ValueError:
 math.isnan(Decimal('snan'))
 math.isnan(Decimal('nan123'))
(Tested on Python 3.2 and 3.3.0a1)

Yep, Decimal.__float__ isn't too sophisticated. Probably it should convert all Decimal quiet NaNs (at least) to float NaNs, keeping the sign if possible but discarding any payload.
Not so sure about signaling NaNs, though; I think it would be fine for those to continue to raise ValueError (on the basis that doing pretty much anything with a signaling NaN should give an exception).

Attached is a patch for decimal.py, and test_decimal.py. I cannot provide a patch for the C decimal implementation, sorry.
Following Mark's suggestion, my patch keeps the sign but discards the payload for quiet NANs, and raises ValueError for signalling NANs.
(I'm ambivalent about signalling NANs raising ValueError, but I guess that's better than having snan silently converted to a quiet nan.)

I think math.isnan('snan') probably should not raise. Decimal('snan').is_nan()
just returns true and I am under the impression that IEEE 754 specifies the 
same. I have to admit though that I just consulted Wikipedia for the latter:
"The predicate isNaN(x) determines if a value is a NaN and never signals an
 exception, even if x is a signaling NaN."

> Decimal('snan').is_nan()
> just returns true and I am under the impression that IEEE 754 specifies > the same.
Sure, but IEEE 754 also specifies that math.sqrt(<signalling nan>) should signal. Since both math.sqrt and math.isnan are going through __float__, we can't keep everyone happy here.
The question for me is really what __float__ should do. IEEE 754 doesn't help here, since it doesn't cover decimal floating-point <-> binary floating-point conversions.

> IEEE 754 doesn't help here, since it doesn't cover decimal
> floating-point <-> binary floating-point conversions.
OTOH, IEEE 754 *does* cover floating-point to int conversions (5.4.1, 5.8): those fall under 'general-computational operations', and as such should signal when given an sNaN (6.2: "Signaling NaNs shall be reserved operands that, under default exception handling, signal the invalid operation exception (see 7.2) for every general-computational and signaling-computational operation except for the conversions described in 5.12. For non-default treatment, see 8."). It feels to me as though decimal -> binary conversions should follow the same pattern.

On 03/08/12 22:06, Mark Dickinson wrote:
>
>> Decimal('snan').is_nan() just returns true and I am under the impression
>> that IEEE 754 specifies the same.
>
> Sure, but IEEE 754 also specifies that math.sqrt(<signalling nan>) should
> signal. Since both math.sqrt and math.isnan are going through __float__,
> we can't keep everyone happy here.
Is it necessarily a given that math.isnan *must* go through __float__? If it 
were written in Python, it would be a simple matter of including
if isinstance(x, Decimal) and x.isnan(): return True
before the conversion to float. By I have no idea whether that is practical in 
the math module.
> The question for me is really what __float__ should do. IEEE 754 doesn't
> help here, since it doesn't cover decimal floating-point<-> binary
> floating-point conversions.
Until such time that floats officially support snans, I think ValueError is 
the right behaviour.

> before the conversion to float. By I have no idea whether that is
> practical in the math module.
That's a much bigger discussion: as it is, most of the math module functions just provide simple wrappers around the system math library, which deals purely with floats. *Some* of the math module functions have been made more generic, like floor and ceil (which look for special __floor__ and __ceil__ methods), but isnan isn't one of those: like most of the math module functions, it simply converts whatever it's given to float, then passes it on to the system library and returns whatever it gets back.
Changing that would be feature request targeted at 3.4 or later; I'm not saying that it shouldn't be considered, but it doesn't belong in this issue.

Why not add a is_nan() method to float numbers instead?

> OTOH, IEEE 754 *does* cover floating-point to int conversions (5.4.1, 5.8): those fall under 'general-computational operations', and as such should signal when given an sNaN.
That sounds good. Let's keep the ValueError then. We could consider
InvalidOperation like with Decimal('snan').to_integral(), but that's
probably confusing in the context of the math module.

> Why not add a is_nan() method to float numbers instead?
Do you mean replacing math.isnan(x) by x.is_nan() to avoid the issue
altogether? I'm not sure that's possible given that math just wraps
the C library.

> > Why not add a is_nan() method to float numbers instead?
> 
> Do you mean replacing math.isnan(x) by x.is_nan() to avoid the issue
> altogether? I'm not sure that's possible given that math just wraps
> the C library.
Yup. By calling x.is_nan() you would by construction get an
implementation that's correct for x's type. If x is a float, it would
obviously re-use math.isnan() (or have a similar implementation).

> Why not add a is_nan() method to float numbers instead?
That could work. The duplication of float.is_nan and math.isnan (not to mention the different spellings) would be a bit ugly, but perhaps worth it. It would make sense to add float.is_infinite and (possibly) float.is_finite methods at the same time.
Looks like we've got two separate issues here, that should probably be split into two separate bug reports. The first issue is that Decimal.__float__ is brain-dead when it comes to NaNs with payloads; I consider that a clear bug, and Steven's patch fixes it nicely for the Python version of decimal. The second has to do with finding a nice type-agnostic way of determing whether something is a NaN---anyone mind if I open a separate issue for this?
W.r.t. the first issue: Steven, thanks for the patch; looks fine to me at first glance.
Two questions: (1) What would you think about raising ValueError explicitly for the signaling NaN case rather than falling back to the ValueError coming from the string-to-float conversion. I think the intentions of the code would be a little clearer that way; and we get to choose a more informative error message that way, too. (2) Should we apply the fix to 2.7 and/or 3.2 as well?
I'll look at extending Steven's fix to the cdecimal code, unless Stefan really wants to do it (which would be fine with me :-).

Mark Dickinson <report@bugs.python.org> wrote:
> Looks like we've got two separate issues here, that should probably be
> split into two separate bug reports. The first issue is that
> Decimal.__float__ is brain-dead when it comes to NaNs with payloads;
> I consider that a clear bug, and Steven's patch fixes it nicely for
> the Python version of decimal.
If we are viewing the whole issue in terms of decimal -> float conversion,
I'm not so sure anymore: Not all Decimal payloads have a meaning for floats
(and payloads get lost in the conversion).
On the other hand it doesn't matter since I doubt anyone is using payloads. :)
> The second has to do with finding a nice type-agnostic way of determing
> whether something is a NaN---anyone mind if I open a separate issue for this?
Yes, that should probably be another issue.
> Two questions: (1) What would you think about raising ValueError explicitly
> for the signaling NaN case rather than falling back to the ValueError coming
> from the string-to-float conversion.
If we use your latest rationale for raising in case of Decimal('snan').__float__(),
I think that indeed __float__() should raise like Decimal.to_integral() does
for example.
If we are aiming for sNaN support in floats in the long term and at some point
allow carrying over sNaNs from decimal to float, then perhaps the error message
could say that sNaN conversion is currently not supported.
> (2) Should we apply the fix to 2.7 and/or 3.2 as well?
Yes, I think so.
> I'll look at extending Steven's fix to the cdecimal code, unless Stefan
> really wants to do it (which would be fine with me :-).
Please go ahead! For this year, I've seen more than enough of _decimal.c
already. :)

On 05/08/12 03:45, Mark Dickinson wrote:
> It would make sense to add float.is_infinite and (possibly) float.is_finite
> methods at the same time.
If you don't add is_finite, you know someone is going to express surprise that 
it wasn't already done. Just as happened with math.isfinite :)
http://bugs.python.org/issue9165#msg109326
> The second has to do with finding a nice
> type-agnostic way of determing whether something is a NaN---anyone mind if
> I open a separate issue for this?
Please do.
> Two questions: (1) What would you think about raising ValueError
> explicitly for the signaling NaN case [...] (2) Should we apply
> the fix to 2.7 and/or 3.2 as well?
Agree to both. I think this counts as a bug report and not a new feature.
> I'll look at extending Steven's fix to the cdecimal code
Thank you :)

-0.5 on making the math module functions aware of decimals. 
The math module was originally conceived as thin wrapper around the C math library. Subsequently, it has had feature creep (I'm guilty of putting the integer factorial method in this module). I don't think further creep and loss of focus is warranted.
Making some functions decimal aware and others not is probably not a good idea.
Also, if someone is using decimals, they are better of using the methods supplied in that module (those have at least passed the huge battery of tests for compliance with the spec).

Here's an updated patch that extends Steven's fix to the C code.

The patch looks good in every detail. +1 for committing.

New changeset 49014c59b31f by Mark Dickinson in branch 'default':
Issue #15544: Fix Decimal.__float__ to work with payload-carrying NaNs.
http://hg.python.org/cpython/rev/49014c59b31f 

New changeset a931e44ffbe1 by Mark Dickinson in branch '3.2':
Issue #15544: Fix Decimal.__float__ to work with payload-carrying NaNs.
http://hg.python.org/cpython/rev/a931e44ffbe1 

New changeset 5dd5f824428c by Mark Dickinson in branch '2.7':
Issue #15544: Fix Decimal.__float__ to work with payload-carrying NaNs.
http://hg.python.org/cpython/rev/5dd5f824428c 

Fixed. (I managed to mess up the commit to 3.2 and break all the buildbots :-(. I think it's okay now.)
Thanks Steven for the report and patch! (And thanks Stefan for reviewing.)