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
math.isnan fails with some Decimal NaNs #59749
Comments
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() "The predicate isNaN(x) determines if a value is a NaN and never signals an |
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. |
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:
Is it necessarily a given that math.isnan *must* go through __float__? If it if isinstance(x, Decimal) and x.isnan(): return True before the conversion to float. By I have no idea whether that is practical in
Until such time that floats officially support snans, I think ValueError is |
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? |
That sounds good. Let's keep the ValueError then. We could consider |
Do you mean replacing math.isnan(x) by x.is_nan() to avoid the issue |
Yup. By calling x.is_nan() you would by construction get an |
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:
If we are viewing the whole issue in terms of decimal -> float conversion, On the other hand it doesn't matter since I doubt anyone is using payloads. :)
Yes, that should probably be another issue.
If we use your latest rationale for raising in case of Decimal('snan').__float__(), If we are aiming for sNaN support in floats in the long term and at some point
Yes, I think so.
Please go ahead! For this year, I've seen more than enough of _decimal.c |
On 05/08/12 03:45, Mark Dickinson wrote:
If you don't add is_finite, you know someone is going to express surprise that http://bugs.python.org/issue9165#msg109326
Please do.
Agree to both. I think this counts as a bug report and not a new feature.
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': |
New changeset a931e44ffbe1 by Mark Dickinson in branch '3.2': |
New changeset 5dd5f824428c by Mark Dickinson in branch '2.7': |
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.) |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: