bugGNU Octave - Bugs: bug #44206, test complex.tst fails in...

 
 

bug #44206: test complex.tst fails in MXE-octave

Submitter:  Avinoam Kalma <avinoam>
Submitted:  Sat 07 Feb 2015 09:14:10 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Avinoam Open/Closed:  * Closed
Release:  * dev Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 20 Mar 2015 02:57:37 PM UTC, comment #8: 

Works for me in Windows XP.  Marking report fixed and closing.

Rik <rik5>
Group administrator
Thu 19 Mar 2015 04:31:07 PM UTC, comment #7: 

I checked in this change:

http://hg.savannah.gnu.org/hgweb/octave/rev/40b2c4323c16

It seemed to work before the --disable-float-truncate change, so I think it will work properly no matter what the default is.

John W. Eaton <jwe>
Group administrator
Thu 19 Mar 2015 04:11:09 PM UTC, comment #6: 

Ah, I see, I didn't try to step into the comparison operator.

However, the comparison operator doesn't handle NaN the same as the nan_ascending_compare.  Why not?  Should it?

John W. Eaton <jwe>
Group administrator
Thu 19 Mar 2015 03:49:06 PM UTC, comment #5: 

I have also wondered why we have two functions that do sorting, rather than just one.  I think long term that is a problem as we are maintaining two pieces of code that can go out of sync, as they have already done here.

The sort routine in oct-sort.cc is fine because it defines a generic sort operation but the definition of the comparison operators like '>' or '<' is overloaded.  The overloaded definition of comparison operators for complex values is in oct-cmplx.h.  Extracting the definition:


#define DEF_COMPLEXR_COMP(OP, OPS) \
template <class T> \
inline bool operator OP (const std::complex<T>& a, const std::complex<T>& b) \
{ \
  FLOAT_TRUNCATE const T ax = std::abs (a); \
  FLOAT_TRUNCATE const T bx = std::abs (b); \
  if (ax == bx) \
    { \
      FLOAT_TRUNCATE const T ay = std::arg (a); \
      FLOAT_TRUNCATE const T by = std::arg (b); \
      if (ay == static_cast<T> (-M_PI)) \
        { \
          if (by != static_cast<T> (-M_PI)) \
            return static_cast<T> (M_PI) OP by; \
        } \
      else if (by == static_cast<T> (-M_PI)) \
        { \
          return ay OP static_cast<T> (M_PI); \
        } \
      return ay OP by; \
    } \
  else \
    return ax OPS bx; \
} \


As you can see, it already uses FLOAT_TRUNCATE and comparisons to static compiler-time values.

I think the short term fix is to add FLOAT_TRUNCATE, and maybe static_cast calls, to Array-C.cc and Array-fC.cc.  Longer term, I would ditch one of the sort routines and consolidate.

Rik <rik5>
Group administrator
Thu 19 Mar 2015 03:06:30 PM UTC, comment #4: 

In the sort function, Octave is just using the simple comparison operator that is defined in oct-sort.cc.  That doesn't account for NaNs or the arg of a complex value.  But the issorted function is using the nan_ascending_compare function that is defined in Array-C.cc:


static bool
nan_ascending_compare (const Complex& x, const Complex& y)
{
  bool retval = false;

  if (xisnan (y))
    retval = (! xisnan (x));
  else
    {
      double xabs = std::abs (x);
      double yabs = std::abs (y);

      if (xabs < yabs)
        retval = true;
      else if (xabs == yabs)
        {
          double xarg = arg (x);
          double yarg = arg (y);

          xarg = (xarg == -M_PI) ? M_PI : xarg;
          yarg = (yarg == -M_PI) ? M_PI : yarg;

          retval = xarg < yarg;
        }
      else
        retval = false;
    }

  return retval;
}


The comparison that fails is


(-1,-0) < (-1,0)


It should return false, but returns true on Windows systems, apparently because the xarg and yarg values that are computed are stored in registers that end up not being exactly equal to M_PI or -M_PI.  It's hard to observe because the values are optimized out so I can't look at them with gdb and attempting to print them causes the compiler to store them, which makes the function "work".

I'm checking now to see whether declaring xarg and yarg as volatile will fix the problem.  I assume it will, but this doesn't seem like a great fix in any case because we are still relying on exact comparison of a value returned from a function with a compile-time constant.  Hmm.

Another question is why the sort function is not using the special nan_ascending_compare function.  Shouldn't it be using that one so that we get complex comparisons right?  And, in any case, shouldn't the comparison function that is used by sort be consistent with the one used by issorted?

John W. Eaton <jwe>
Group administrator
Wed 18 Mar 2015 06:19:32 PM UTC, comment #3: 

All tests (6 of 6) do pass on a 64-bit Octave-4.0.0-rc1 (no 64-bit indexing, just --enable-windows-64, although I suspect it'll work with 64-bit idx as well)



Philip Nienhuis <philipnienhuis>
Group Member
Sun 08 Feb 2015 06:24:39 AM UTC, comment #2: 

Confirmed.

Rik <rik5>
Group administrator
Sat 07 Feb 2015 09:16:08 PM UTC, comment #1: 


 x = [complex(-1,0), complex(-1,-0), i, -i, 1]
x =
  -1 + 0i  -1 - 0i   0 + 1i  -0 - 1i   1 + 0i
 xs = sort (x);
xs =
  -0 - 1i   1 + 0i   0 + 1i  -1 + 0i  -1 - 0i
>> issorted (xs)
ans = 0
 assert (issorted (xs));
!!!!! test failed
assert (issorted (xs)) failed


Under unix this test does not fail

Avinoam Kalma <avinoam>
Group Member
Sat 07 Feb 2015 09:14:10 PM UTC, original submission:  


Avinoam Kalma <avinoam>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by avinoam (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-03-20 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2015-03-08 rik5 Dependencies- bugs #44445 is dependent
    2015-02-08 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code