bugGNU Octave - Bugs: bug #50256, Wrong data type for rand...

 
 

bug #50256: Wrong data type for rand ('twister')

Submitter:  Avinoam Kalma <avinoam>
Submitted:  Wed 08 Feb 2017 06:41:17 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  jwe
Originator Name:  Avinoam Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 11 Mar 2018 06:35:08 PM UTC, comment #10: 

It works, thanks.

Closing as fixed.

Avinoam Kalma <avinoam>
Group Member
Sat 10 Mar 2018 12:52:51 AM UTC, comment #9: 

I pushed the change here (http://hg.savannah.gnu.org/hgweb/octave/rev/8bb0251fcfde).  I also cranked down the tolerances on the BIST tests as they were quite high (1e-6) when they appear to only need a few eps worth of margin.  We can relax them if they start to show failures.

Rik <rik5>
Group administrator
Fri 09 Mar 2018 07:33:41 PM UTC, comment #8: 

To eliminate the OCTAVE_LOCAL_BUFFER copy, how about something like this?


void
octave_rand::set_internal_state (const uint32NDArray& s)
{
  octave_idx_type len = s.numel ();

  const uint32_t *psdata = reinterpret_cast<const uint32_t *> (s.data ());

  if (len == MT_N + 1 && psdata[MT_N] <= MT_N && psdata[MT_N] > 0)
    oct_set_state (psdata);
  else
    oct_init_by_array (psdata, len);
}


John W. Eaton <jwe>
Group administrator
Fri 09 Mar 2018 07:05:45 PM UTC, comment #7: 

@jwe: Thanks, I'll work on fixing the BIST tests.

Rik <rik5>
Group administrator
Fri 09 Mar 2018 06:46:48 PM UTC, comment #6: 

I don't know of a good way around the reinterpret cast unless you are OK with making a copy or defining oct_get_state to work on an array of octave_int32 objects instead of uint32_t.  But the cast should be OK because the data for octave_int32 is a single uint32_t object only, and that should not change.  We only define this data type so that we can enforce Matlab integer semantics.  Is there a better way to handle that?

John W. Eaton <jwe>
Group administrator
Fri 09 Mar 2018 05:39:42 PM UTC, comment #5: 

This is relatively easy, replacing declarations of ColumnVector with uint32NDArray.  See the attached patch.

I'm not completely happy with it.  The first issue is that I used a reinterpret_cast because I couldn't make the compiler understand that fortran_vec() should be returning a pointer to uint32 data.


uint32NDArray
octave_rand::get_internal_state (void)
{
  uint32NDArray s (dim_vector (MT_N + 1, 1));

  oct_get_state (reinterpret_cast<uint32_t *> (s.fortran_vec ()));

  return s;
}


The second issue was the set_internal_state routine.  It should be simple to get rid of the OCTAVE_LOCAL_BUFFER and just use s.data() to get a constant pointer to the data.  But I couldn't make it work.


void
octave_rand::set_internal_state (const uint32NDArray& s)
{
  octave_idx_type len = s.numel ();
  octave_idx_type n = (len < MT_N + 1 ? len : MT_N + 1);

  OCTAVE_LOCAL_BUFFER (uint32_t, tmp, MT_N + 1);

  for (octave_idx_type i = 0; i < n; i++)
    tmp[i] = s.xelem (i);

  if (len == MT_N + 1 && tmp[MT_N] <= MT_N && tmp[MT_N] > 0)
    oct_set_state (tmp);
  else
    oct_init_by_array (tmp, len);
}


If things can be made to work then 4 BIST tests need updating.  One of the good things is that this brings us much closer to Matlab.  When we were converting doubles to uint32 we used a subroutine with this header:


-// Guarantee reproducible conversion of negative initialization values to
-// random number algorithm.  Note that Matlab employs slightly different rules.
-// 1) Seed saturates at 2^32-1 for any value larger than that.
-// 2) NaN, Inf are translated to 2^32-1.
-// 3) -Inf is translated to 0.
-static uint32_t
-double2uint32 (double d)


The rules mentioned above are exactly those enforced by uint32() in Octave (except for NaN) so comaptibility would be increased.



(file #43500)

Rik <rik5>
Group administrator
Thu 09 Feb 2017 09:35:17 AM UTC, comment #4: 


Matlab:


>> class (rand ('twister'))

ans =

uint32



Anonymous
Thu 09 Feb 2017 01:01:34 AM UTC, comment #3: 

The internal state is already a uint32 array, but then we convert it to a double precision ColumnVector to store it.  Then if we set the state from one of these saved states, we convert it back.  I think we could simplify this by saving the state as a uint32 array instead.  I can work on that.

John W. Eaton <jwe>
Group administrator
Thu 09 Feb 2017 12:36:06 AM UTC, comment #2: 

Does Matlab return uint32?


class (rand ('twister'))


And if it does, the simplest thing would be to cast the double values used internally in C++ to uint32_t at the time a value is returned from rand.

Rik <rik5>
Group administrator
Wed 08 Feb 2017 09:27:41 PM UTC, comment #1: 

Actually, K should be 625x1 uint32, and not 625x1 double.

Avinoam Kalma <avinoam>
Group Member
Wed 08 Feb 2017 06:41:17 PM UTC, original submission:  

While trying to understand bug #42557:


K = rand ('twister');


K should be uint32, but it is double in Octave. How to change it?

Avinoam Kalma <avinoam>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #43500:  rand32.patch added by rik5 (7KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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)
  • -email is unavailable- added by avinoam
  •  

    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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-03-11 avinoam StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-03-10 rik5 StatusNeed Info Ready For Test
        Release4.2.0 dev
    2018-03-09 jwe Releasedev 4.2.0
    2018-03-09 rik5 Release4.2.0 dev
    2018-03-09 rik5 Attached File- Added rand32.patch, #43500
    2017-02-09 jwe Assigned toNone jwe
    2017-02-09 rik5 StatusNone Need Info
    2017-02-08 avinoam Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code