[Changeset] Re: Single Precision versus double precision NA

[David Bateman]

David Bateman wrote:
> Jaroslav Hajek wrote:
>> I guess that backward compatibility is not a big issue here because
>> NAs are used much less in Octave than in R (I think Matlab doesn't
>> support them). So I'd vote for changing the value so that the
>> conversion works. If ever R gets single precision, it will face the
>> same issue (and that's going to be more painful, as NAs are heavily
>> used in R).
>>
>> Btw., there are still issues with NaN as well. It seems that both
>> Octave and Matlab do use 0xfff8 0000 0000 0000, i.e. NaN with signbit
>> set. This makes Octave output -NaN instead of NaN in some cases (e.g.
>> NaN*i). Matlab apparently ignores the possibility of signed NaN
>> completely. Some time ago I submitted a patch that makes Octave use
>> the positive NaN as the default (which seems more sensible). It seems
>> it was never applied, presumably because of breaking the compatibility
>> (?)
>>
>>
>>   
> Frankly this is my preferred solution as well. Though the issue of
> existing uses of saved NA values will need to be explicitly taken into
> account. That can easily be done in the isna functions where both the
> old and new values are checked.
> 
> However, I have one issue with doing it in this manner. That is ensuring
> that the double to single conversion of the NA value and visa-versa
> works with the compilers we will be using. I used the code
> 
> include <stdio.h>
> 
> typedef union
> {
>   float value;
>   unsigned int word;
> } lo_ieee_float;
> 
> typedef union
> {
>   double value;
>   unsigned int word[2];
> } lo_ieee_double;
> 
> int main (void)
> {
>   lo_ieee_float vf;
>   lo_ieee_double vd;
>   vf.word = 0x7FC207A2;
>   vd.value = vf.value;
> 
> #ifdef HAVE_LITTLE_ENDIAN
>   if (vd.word[0] != 0x7FF840F4 || vd.word[1] != 0x40000000)
> #else
>   if (vd.word[1] != 0x7FF840F4 || vd.word[0] != 0x40000000)
> #endif
>     printf("ERROR converting single NA to double\n");
>   else
>     printf("single NA to double conversion OK\n");
> 
>   vf.value = vd.value;
> 
>   if (vf.word != 0x7FC207A2)
>     printf("ERROR converting double NA to single\n");
>   else
>     printf("double NA to single conversion OK\n");
> 
>   return (0);
> }
> 
> to test this idea with gcc 4.2.2 on a x86 based machine and the
> conversion worked correctly. However, does this work for other platforms
> and compilers.. It would be good to check as least one Big endian (note
> that the ifdef above should be changed) and the windows platforms..
> Anyone want to run these tests for me?


Ok, then the attached changeset addresses this by changing the NA values
as above. I still have no confirmation that the above code worked as
expected for other machines than my own. However I added a couple of
tests that check that the translation of NA values from single to double
precision and visa-versa is done correctly and so if make check is run
on different systems we'll quickly find if there is an issue with this
change. I also added code to data-conv.cc (read_doubles) to check for
the old NA value and convert it to the new one. So there is forward
compatibility for the NA values saved with older versions of Octave.
However, NA values saved after this change won't be loaded correctly
with older version of Octave. In any case if the conversion in this
changeset works correctly for other platforms, OS and compilers then
this is probably the simplest way to address this issue.

D.


# HG changeset patch
# User David Bateman <address@hidden>
# Date 1212695894 -7200
# Node ID ae037085ab89a5f1a8568fab83e5a1fb7ede71d2
# Parent  53d185605493e517c9e7d98ea0f4d2b9fc5e8dfb
Change NA value to support single to double precision conversion

diff --git a/liboctave/ChangeLog b/liboctave/ChangeLog
--- a/liboctave/ChangeLog
+++ b/liboctave/ChangeLog
@@ -1,3 +1,21 @@ 2008-06-02  David Bateman  <address@hidden
+2008-06-05  David Bateman  <address@hidden>
+
+       * lo-ieee.h (LO_IEEE_NA_HW, LO_IEEE_NA_LW, LO_IEEE_NA_FLOAT):
+       Change definition so cast from single to double and visa versa
+       maintains NA value.
+       (LO_IEEE_NA_HW_OLD, LO_IEEE_NA_LW_OLD): Keep old values.
+       (extern OCTAVE_API int __lo_ieee_is_old_NA (double)): Function to
+       detect old NA value.
+       (extern OCTAVE_API double __lo_ieee_replace_old_NA (double)):
+       Function to replace old NA value with new new.
+       * lo-cieee.c (int __lo_ieee_is_old_NA (double)): Function to
+       detect old NA value.
+       (double __lo_ieee_replace_old_NA (double)): Function to replace
+       old NA value with new new.
+       * data-conv.cc (void read_doubles(std::istream&, double *, 
+       save_type, int, bool, octave_mach_info::float_format)): Test if
+       loaded NA values is the old representation and replace it.
+       
 2008-06-02  David Bateman  <address@hidden>
 
        * fCmplxDET.cc (FloatComplexDET::value_will_overflow,
diff --git a/liboctave/data-conv.cc b/liboctave/data-conv.cc
--- a/liboctave/data-conv.cc
+++ b/liboctave/data-conv.cc
@@ -34,6 +34,7 @@ along with Octave; see the file COPYING.
 #include "byte-swap.h"
 #include "data-conv.h"
 #include "lo-error.h"
+#include "lo-ieee.h"
 
 template void swap_bytes<2> (volatile void *, int);
 template void swap_bytes<4> (volatile void *, int);
@@ -1048,8 +1049,13 @@ read_doubles (std::istream& is, double *
       break;
 
     case LS_DOUBLE: // No conversion necessary.
-      is.read (reinterpret_cast<char *> (data), 8 * len);
-      do_double_format_conversion (data, len, fmt);
+      {
+       is.read (reinterpret_cast<char *> (data), 8 * len);
+       do_double_format_conversion (data, len, fmt);
+
+       for (int i = 0; i < len; i++)
+         data[i] = __lo_ieee_replace_old_NA (data[i]);
+      }
       break;
 
     default:
diff --git a/liboctave/lo-cieee.c b/liboctave/lo-cieee.c
--- a/liboctave/lo-cieee.c
+++ b/liboctave/lo-cieee.c
@@ -150,10 +150,33 @@ __lo_ieee_is_NA (double x)
 #if defined (HAVE_ISNAN)
   lo_ieee_double t;
   t.value = x;
-  return (isnan (x) && t.word[lo_ieee_lw] == LO_IEEE_NA_LW) ? 1 : 0;
-#else
-  return 0;
-#endif
+  return (isnan (x) && t.word[lo_ieee_hw] == LO_IEEE_NA_HW 
+         && t.word[lo_ieee_lw] == LO_IEEE_NA_LW) ? 1 : 0;
+#else
+  return 0;
+#endif
+}
+
+int
+__lo_ieee_is_old_NA (double x)
+{
+#if defined (HAVE_ISNAN)
+  lo_ieee_double t;
+  t.value = x;
+  return (isnan (x) && t.word[lo_ieee_lw] == LO_IEEE_NA_LW_OLD 
+         && t.word[lo_ieee_hw] == LO_IEEE_NA_HW_OLD) ? 1 : 0;
+#else
+  return 0;
+#endif
+}
+
+double
+__lo_ieee_replace_old_NA (double x)
+{
+  if (__lo_ieee_is_old_NA (x))
+    return lo_ieee_na_value ();
+  else
+    return x;
 }
 
 int
diff --git a/liboctave/lo-ieee.h b/liboctave/lo-ieee.h
--- a/liboctave/lo-ieee.h
+++ b/liboctave/lo-ieee.h
@@ -64,9 +64,12 @@ typedef union
   unsigned int word;
 } lo_ieee_float;
 
-#define LO_IEEE_NA_HW 0x7ff00000
-#define LO_IEEE_NA_LW 1954
-#define LO_IEEE_NA_FLOAT   0x7f8207a2
+#define LO_IEEE_NA_HW_OLD 0x7ff00000
+#define LO_IEEE_NA_LW_OLD 1954
+#define LO_IEEE_NA_HW 0x7FF840F4
+#define LO_IEEE_NA_LW 0x40000000
+#define LO_IEEE_NA_FLOAT   0x7FC207A2
+ 
 
 extern OCTAVE_API void octave_ieee_init (void);
 
@@ -85,7 +88,9 @@ extern OCTAVE_API int __lo_ieee_isinf (d
 extern OCTAVE_API int __lo_ieee_isinf (double x);
 
 extern OCTAVE_API int __lo_ieee_is_NA (double);
+extern OCTAVE_API int __lo_ieee_is_old_NA (double);
 extern OCTAVE_API int __lo_ieee_is_NaN_or_NA (double) GCC_ATTR_DEPRECATED;
+extern OCTAVE_API double __lo_ieee_replace_old_NA (double);
 
 extern OCTAVE_API double lo_ieee_inf_value (void);
 extern OCTAVE_API double lo_ieee_na_value (void);
diff --git a/src/ChangeLog b/src/ChangeLog
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@ 2008-06-05  John W. Eaton  <address@hidden
+2008-06-05  David Bateman  <address@hidden>
+
+       * data.cc (FNA): Add tests for conversion of single to double NA
+       values.
+
 2008-06-05  John W. Eaton  <address@hidden>
 
        * graphics.cc (properties::update_normals):
diff --git a/src/data.cc b/src/data.cc
--- a/src/data.cc
+++ b/src/data.cc
@@ -3819,6 +3819,13 @@ to the special constant used to designat
                      lo_ieee_float_na_value (), "NA");
 }
 
+/*
+
+%!assert(single(NA('double')),NA('single'))
+%!assert(double(NA('single')),NA('double'))
+
+ */
+
 DEFUN (false, args, ,
   "-*- texinfo -*-\n\
 @deftypefn {Built-in Function} {} false (@var{x})\n\