classification
Title: math.copysign(1., float('nan')) returns -1.
Type: enhancement Stage: needs patch
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: eric.smith, loewis, mark.dickinson, mattip, python-dev, serhiy.storchaka
Priority: low Keywords: patch

Created on 2012-04-07 18:43 by mattip, last changed 2012-08-24 19:32 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue14521.patch mark.dickinson, 2012-04-09 17:02 review
math_patch2.txt mattip, 2012-04-09 20:57 issue14521_with_tests.patch review
Messages (24)
msg157746 - (view) Author: mattip (mattip) * Date: 2012-04-07 18:43
Python 2.7.2 (default, Jun 12 2011, 15:08:59) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import math
>>> math.copysign(1., float('inf'))
1.0
>>> math.copysign(1., float('-inf'))
-1.0
>>> math.copysign(1., float('nan'))
-1.0
>>> math.copysign(1., float('-nan'))
1.0
>>>
msg157748 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-04-07 19:06
This is a near duplicate of issue7281. Most likely, copysign is behaving correctly, and it's already the float conversion that errs.

For struct.pack('d', float('nan')), I get '\x00\x00\x00\x00\x00\x00\xf8\xff'; 
for -nan, I get '\x00\x00\x00\x00\x00\x00\xf8\x7f'; 
ISTM that this has the sign bits switched.
msg157761 - (view) Author: mattip (mattip) * Date: 2012-04-07 22:26
It appears that microsoft decided NAN will be represented by '\x00\x00\x00\x00\x00\x00\xf8\xff', which has the sign bit set. 
Compiling this c code with visual 9.0 gives the correct answers for the first value, and a mess for the second:

#include <math.h>
#include <stdio.h>
#include <float.h>

int main( void ) {
   unsigned long nan[2]={0xffffffff, 0x7fffffff};
   double g;
   double z, zn;
   int i;
   for (i=0;i<2; i++)
   {    
       g = *( double* )(nan+i);
       printf( "g( %g ) is NaN, _isnan(g) %d\n", g, _isnan(g) );
       z = _copysign(-3, g);
       zn = _copysign(-3, -g);
       printf("z=%g, zn=%g\n", z, zn);
   }
   return 0;
}

This corresponds with loewis 's observation.
msg157765 - (view) Author: mattip (mattip) * Date: 2012-04-08 00:21
Sorry for the garbage c code, the comment however is correct: Py_NAN is created with the sign bit set (on windows), and then _copysign on windows uses the sign bit to determine the output, which results in the wrong answer.
msg157779 - (view) Author: mattip (mattip) * Date: 2012-04-08 07:10
I was going to add a test for this to Lib/test/test_math.py, but found this comment:
        # copysign(INF, NAN) may be INF or it may be NINF, since
        # we don't know whether the sign bit of NAN is set on any
        # given platform. 

I would try to claim this is fixable, by this patch:

--- Include\pymath.h.orig       Sun Apr 08 10:02:37 2012
+++ Include\pymath.h    Sun Apr 08 10:02:41 2012
@@ -150,7 +150,7 @@
  * doesn't support NaNs.
  */
 #if !defined(Py_NAN) && !defined(Py_NO_NAN)
-#define Py_NAN (Py_HUGE_VAL * 0.)
+#define Py_NAN abs(Py_HUGE_VAL * 0.)
 #endif

 /* Py_OVERFLOWED(X)

Should I rework the tests to reflect this and submit a patch?
msg157796 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-04-08 16:30
Hmm.  I don't see this as a bug:  the sign of a nan isn't really all that meaningful anyway, and this doesn't (AFAIK) contradict any documentation.

On the other hand, given that most other aspects of floating-point are now reasonably portable across platforms (or at least those platforms that support IEEE 754), it may make sense to standardise this too, as a minor improvement for Python 3.3.

The patch does not look correct to me:  did you mean 'fabs' rather than 'abs'?  Even then, the C standard says nothing (not even in annex F) about how fabs should behave when applied to a NaN.

Instead, if the platform supports IEEE 754 (e.g., if the short float repr code is in use), we could hard-code a NaN bit representation.  (Even though there are 2**53-2 distinct bit patterns representing NaNs, there are nevertheless a couple of obvious ones to choose:  there are exactly two quiet NaNs that don't arise by silencing a signaling NaN.  The one with the sign bit cleared would be an obvious choice;  I think it's the negation of the one that Intel uses by default, which does indeed have its sign bit set.)
msg157807 - (view) Author: mattip (mattip) * Date: 2012-04-08 19:25
You are correct, the patch should use fabs
I would go with a standard, cross-platform definition of Py_NAN so that pickled objects could be opened by other platforms. Would this patch be better? It's more complicated as I needed to cast the repr of Py_NAN to a unsigned char[]. It passes the tests in test.test_math and handles the copysign in a more intuitive way

>>> math.copysign(1., float('nan')) => 1. on win32, microsoft compiler

diff -r efeca6ff2751 Include/pymath.h
--- a/Include/pymath.h  Thu Apr 05 22:51:00 2012 +0200
+++ b/Include/pymath.h  Sun Apr 08 22:20:16 2012 +0300
@@ -152,8 +152,13 @@
  * doesn't support NaNs.
  */
 #if !defined(Py_NAN) && !defined(Py_NO_NAN)
+#if DBL_MANT_DIG == 53 /* ieee 754 doubles */
+extern double * _Py_NAN;
+#define Py_NAN (*_Py_NAN)
+#else
 #define Py_NAN (Py_HUGE_VAL * 0.)
 #endif
+#endif

 /* Py_OVERFLOWED(X)
  * Return 1 iff a libm function overflowed.  Set errno to 0 before calling
diff -r efeca6ff2751 Modules/_math.c
--- a/Modules/_math.c   Thu Apr 05 22:51:00 2012 +0200
+++ b/Modules/_math.c   Sun Apr 08 22:20:16 2012 +0300
@@ -24,6 +24,10 @@
 static const double two_pow_p28 = 268435456.0; /* 2**28 */
 static const double zero = 0.0;

+#if DBL_MANT_DIG == 53 /* ieee 754 doubles */
+static const unsigned char _Py_NAN_as_char[8] = {0, 0, 0, 0, 0, 0, 0xf8, 0x7f};
+extern double * _Py_NAN = (double *)(_Py_NAN_as_char);
+#endif
 /* acosh(x)
  * Method :
  *      Based on
msg157859 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-04-09 16:25
Thanks for the updated patch!  (BTW, you can attach patches as files to the issue rather than writing them inline.)

Yes, this patch is more along the lines that I was thinking of.  There are some issues, though:  (1) we need to deal with endianness issues (including the ARM mixed-endian case).  (2) It looks to me as though the (double *) cast violates strict aliasing rules;  gcc's optimizations can do nasty things in this area.

Rather than reinventing the wheel, we should use the same mechanisms as are already in Python's version of dtoa.c (e.g., see the use of the union to deal with aliasing issues);  we may even be able to steal bits of David Gay's original code directly.

I'll try to find time to look at this in the near future.  I'm still not convinced that anything really needs to change here, though.

I don't understand your comment about pickled objects;  as far as I'm aware there aren't any issues with transferring pickled NaNs from one system to another.
msg157862 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-04-09 17:02
Here's an example based on the dtoa.c code.  It only changes the return value of float('nan'), and doesn't affect any other existing uses of the Py_NAN macro.  It needs tests.
msg157873 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-04-09 18:07
May be it would be more reasonable if math.copysign(1., float('nan')) return a float('nan')?
msg157874 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-04-09 18:13
> May be it would be more reasonable if math.copysign(1., float('nan')) 
> return a float('nan')?

-1.  That would go against all the existing standards.
msg157875 - (view) Author: mattip (mattip) * Date: 2012-04-09 18:15
Your patch is much more reasonable than mine.
Should I add a test that fails pre-patch and passes with the patch, or one that is skipped pre-patch and passes post-patch? I'm not sure what is accepted in the cpython development cycle
msg157887 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-04-09 19:42
The test should fail pre-patch and pass post-patch. There will be no state in the repository where the patch is present and fails, since it will be committed along with the patch. 

Skipping tests is only ok for tests that lack prerequisites on some systems, and (exceptionally) for tests that document known bugs.
msg157888 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-04-09 19:43
Also, mattip: if you want to contribute some chunk of code (such as the test), please submit a contributor form.
msg157900 - (view) Author: mattip (mattip) * Date: 2012-04-09 20:57
I added tests to the mark.dickinson patch, test.test_math passes.
msg157902 - (view) Author: mattip (mattip) * Date: 2012-04-09 20:59
I also submitted a form. Sorry about the patch name, still learning.
msg157934 - (view) Author: mattip (mattip) * Date: 2012-04-10 03:47
The pickle issue occurs in the numpy module, on windows
>> cPickle.dumps(numpy.array(float('nan')))
yeilds
"cnumpy.core.multiarray\n_reconstruct\np1\n(cnumpy\nndarray\np2\n(I0\ntS'b'\ntRp3\n(I1\n(tcnumpy\ndtype\np4\n(S'f8'\nI0\nI1\ntRp5\n(I3\nS'<'\nNNNI-1\nI-1\nI0\ntbI00\nS'\\x00\\x00\\x00\\x00\\x00\\x00\\xf8\\xff'\ntb."

While this is not a python core issue, it seems that python core could be one possible solution point.
msg157937 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-04-10 06:54
> The pickle issue occurs in the numpy module, on windows

I'm still not clear what the issue is.  Is there something wrong in the output of the pickle example you show?
msg157939 - (view) Author: mattip (mattip) * Date: 2012-04-10 09:13
The pickle output has the sign-bit set. Ignoring the sign-bit, it is unpickled correctly. However math.copysign using this value will now return minus on platforms where copysign(3., float('nan')) is known to work.

Perhaps the whole can of worms should not have been opened in the first place.
Another solution would be to raise a ValueError if copysign(x, float('nan')) is called...
msg157940 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-04-10 09:36
> The pickle output has the sign-bit set. Ignoring the sign-bit, it is 
> unpickled correctly.

Okay, thanks for the clarification.  I just wanted to be clear whether there's a real problem with pickle that should be fixed in 2.7 or not.

Again, I don't see this as a bug:  pickle is transferring the sign bit correctly, so the only issue again is that float('nan') happens to produce a nan whose sign bit is set (depending on platform, compiler options, etc.).  I don't see it as a problem that one can end up with some 'positive' nans and some 'negative' nans on a single system.  As far as I know, none of this violates any documentation or standards (IEEE 754 explicitly places no interpretation on the sign bit of a nan, and makes no guarantees or recommendations about the sign bit of the result of converting the string 'nan').  So at worst, this is a minor violation of user expectations.  Though I do agree that having float('-nan') having its sign bit unset is especially surprising.

So while I don't see this as a bug that should be fixed for 2.7, I'm happy to 'fix' this for Python 3.3, partly for the portability improvement, and partly to avoid having the string-to-float conversions do INF*0 calculations at run time;  that bit's always made me feel uncomfortable.

Thanks for the updated patch;  I'll take a look shortly.
msg159610 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-29 14:32
New changeset c468511fc887 by Mark Dickinson in branch 'default':
Issue #14521: Make result of float('nan') and float('-nan') more consistent across platforms.  Further, don't rely on Py_HUGE_VAL for float('inf').
http://hg.python.org/cpython/rev/c468511fc887
msg159611 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-04-29 14:32
Now fixed.
msg169076 - (view) Author: Roundup Robot (python-dev) Date: 2012-08-24 19:26
New changeset 31a7ff299698 by Mark Dickinson in branch '2.7':
Remove overeager test (don't depend on the sign of a nan;  cf. issue #14521)
http://hg.python.org/cpython/rev/31a7ff299698
msg169078 - (view) Author: Roundup Robot (python-dev) Date: 2012-08-24 19:32
New changeset 121cb9596e7d by Mark Dickinson in branch '3.2':
Remove overeager test (don't depend on the sign of a nan;  cf. issue #14521)
http://hg.python.org/cpython/rev/121cb9596e7d
History
Date User Action Args
2012-08-24 19:32:43python-devsetmessages: + msg169078
2012-08-24 19:26:38python-devsetmessages: + msg169076
2012-04-29 15:57:59mark.dickinsonsetstatus: open -> closed
2012-04-29 14:32:41mark.dickinsonsetresolution: fixed
messages: + msg159611
2012-04-29 14:32:05python-devsetnosy: + python-dev
messages: + msg159610
2012-04-10 09:36:08mark.dickinsonsetmessages: + msg157940
2012-04-10 09:13:06mattipsetmessages: + msg157939
2012-04-10 06:54:11mark.dickinsonsetmessages: + msg157937
2012-04-10 03:47:32mattipsetmessages: + msg157934
2012-04-09 20:59:08mattipsetmessages: + msg157902
2012-04-09 20:57:57mattipsetfiles: + math_patch2.txt

messages: + msg157900
2012-04-09 19:43:26loewissetmessages: + msg157888
2012-04-09 19:42:39loewissetmessages: + msg157887
2012-04-09 18:15:41mattipsetmessages: + msg157875
2012-04-09 18:13:00mark.dickinsonsetmessages: + msg157874
2012-04-09 18:07:36serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg157873
2012-04-09 17:02:58mark.dickinsonsetfiles: + issue14521.patch
keywords: + patch
messages: + msg157862
2012-04-09 16:25:42mark.dickinsonsetpriority: normal -> low

type: behavior -> enhancement
assignee: mark.dickinson
versions: + Python 3.3, - Python 2.7
nosy: + eric.smith

messages: + msg157859
stage: needs patch
2012-04-08 19:25:10mattipsetmessages: + msg157807
2012-04-08 16:30:58mark.dickinsonsetmessages: + msg157796
2012-04-08 14:04:39pitrousetnosy: + mark.dickinson
components: + Interpreter Core, - None
2012-04-08 07:10:27mattipsetmessages: + msg157779
2012-04-08 00:21:57mattipsetmessages: + msg157765
2012-04-07 22:26:34mattipsetmessages: + msg157761
2012-04-07 19:06:24loewissetnosy: + loewis
messages: + msg157748
2012-04-07 18:43:06mattipcreate