bugGNU Octave - Bugs: bug #50776, Octave inconsistent with Matlab...

 
 

bug #50776: Octave inconsistent with Matlab for corner indexing case

Submitter:  Richard <crobar>
Submitted:  Mon 10 Apr 2017 10:12:45 AM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Richard Crozier Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 12 Apr 2017 11:56:54 PM UTC, comment #12: 

I added an xelem function to dim_vector for fast access to the internals (http://hg.savannah.gnu.org/hgweb/octave/rev/77a7f2fecd74).

I'm going to close this bug report, as all issues have been addressed.

Rik <rik5>
Group administrator
Wed 12 Apr 2017 06:17:02 PM UTC, comment #11: 

Rik:  Yeah, Jaroslav loved the cute stuff.

And adding xelem to dim_vector would be fine with me.

John W. Eaton <jwe>
Group administrator
Wed 12 Apr 2017 05:02:08 PM UTC, comment #10: 

Very cute code:


idx_vector::idx_vector (const Array<bool>& bnda)
  : rep (0)
{
  // Convert only if it means saving at least half the memory.
  static const int factor = (2 * sizeof (octave_idx_type));
  octave_idx_type nnz = bnda.nnz ();
  if (nnz <= bnda.numel () / factor)
    rep = new idx_vector_rep (bnda, nnz);
  else
    rep = new idx_mask_rep (bnda, nnz);
}


As a side note, should idx_vector have an xelem method in analogy to the xelem method for Array?

Right now I see only elem().  For example, your new code is



  bool is_nd_vector (void) const
  {
    int num_non_one = 0;

    for (int i = 0; i < ndims (); i++)
      {
        if (elem (i) != 1)
          {
            num_non_one++;

            if (num_non_one > 1)
              break;
          }
      }

    return num_non_one == 1;
  }


But since the loop explicitly runs over the dimensions of the idx_vector there is no need to also run through an assert statement to check that.


  octave_idx_type elem (int i) const
  {
#if defined (OCTAVE_ENABLE_BOUNDS_CHECK)
    assert (i >= 0 && i < ndims ());
#endif
    return rep[i];
  }




Rik <rik5>
Group administrator
Wed 12 Apr 2017 04:39:24 PM UTC, comment #9: 

I didn't change anything about the way index values are stored, just what happens with the dimensions.  Now we handle indices as "vectors" if they have all but one dimension equal to 1.

Yes, logical indices can be stored as vectors but only if the number of true values is small.  See the idx_vector::idx_vector (const Array<bool>& bnda) constructor in idx-vector.cc.

John W. Eaton <jwe>
Group administrator
Wed 12 Apr 2017 04:21:36 PM UTC, comment #8: 

The patch works for me with the reporter's original test!

I wrote "any logical array index is first converted to a column vector" because in my terminology an array is an N-d object which is not a scalar or a vector, what we would traditionally just call a matrix.  Any logical array which is not 1x1, 1xN, or Nx1 is first reshaped to Nx1 before indexing.

In pseudo-code


if (islogical (idx))
  if (! isvector (idx))
    idx = idx(:);
    numeric_idx = find (idx);
  endif
endif
obj(numeric_idx)


One of the advantages to logical indexing is that it takes less memory.  Is that advantage still preserved?  I think it is, but seems worth asking.

For example, for this test case


x = rand (1e4,1);
logical_idx = (x > 0.5);
numeric_idx = find (logical_idx);
whos *idx
Variables in the current scope:

   Attr Name             Size                     Bytes  Class
   ==== ====             ====                     =====  =====
        logical_idx  10000x1                      10000  logical
        numeric_idx   4906x1                      39248  double


As you would expect, numeric indices use a lot more memory because they store everything as 8-byte doubles.

Is there some point where we convert the logical index to a linear index deep in the C++ code?  That would be a bummer.


Rik <rik5>
Group administrator
Wed 12 Apr 2017 02:24:04 PM UTC, comment #7: 
John W. Eaton <jwe>
Group administrator
Wed 12 Apr 2017 01:03:07 PM UTC, comment #6: 

Thanks for the pointer to the blog post.  It doesn't seem to mention anything about N-d "vectors".

And yes, this may be inconsistent with the isvector function, but I still think we should probably be consistent with Matlab here.

"This is really a consequence of the fact that any logical array index is first converted to a column vector. And then the general rule is that for vector/vector operations it is the indexed object whose size is retained. "

Shouldn't that be "converted to a column vector if it is not already a vector"?  Otherwise, wouldn't the result of array(logical_row) be a column vector?  I think it ends up being a row.

John W. Eaton <jwe>
Group administrator
Wed 12 Apr 2017 07:38:18 AM UTC, comment #5: 

oh, and that



>> x = reshape (1:3, 1,1,3);
isvector (x)

ans =

  logical

   0


Richard <crobar>
Wed 12 Apr 2017 07:32:11 AM UTC, comment #4: 

I can confirm JWE's test script produces no output in Matlab R2016b

Richard <crobar>
Tue 11 Apr 2017 11:45:54 PM UTC, comment #3: 

See this article: http://blogs.mathworks.com/loren/2006/08/09/essence-of-indexing/

For logical indexing, an array index is always reshaped to a column vector before indexing.


logical_idx = idx(:);


Thus,


x = reshape (1:9, 3,3)
x =

   1   4   7
   2   5   8
   3   6   9

idx = [true, false;
       true, true];
x(idx)
ans =

   1
   2
   4


This is the generic case of a matrix indexed by a vector so it takes the index's shape.

In your table, a logical index always produces the shape of the indexed object for a vector.


// Logical (mask):
//
//   object   | index    | result orientation
//   ---------+----------+-------------------
//   vector   | anything | indexed object


This is really a consequence of the fact that any logical array index is first converted to a column vector.  And then the general rule is that for vector/vector operations it is the indexed object whose size is retained.

I still think Octave is actually more correct because the text example is an array with three dimensions, not a vector, so it should be an example of matrix/vector indexing.  But, we get to code around their quirks.


Rik <rik5>
Group administrator
Tue 11 Apr 2017 10:39:48 PM UTC, comment #2: 

I think there are some differences between logical and numeric indexing when only one index supplied.

I came up with the following rules:


//
//   object   | index    | result orientation
//   ---------+----------+-------------------
//   anything | colon    | column vector
//
// Logical (mask):
//
//   object   | index    | result orientation
//   ---------+----------+-------------------
//   vector   | anything | indexed object
//   ---------+----------+-------------------
//   array    | vector   | index
//            | other    | column vector
//
// Numeric:
//
//   object   | index    | result orientation
//   ---------+----------+-------------------
//   vector   | vector   | indexed object
//            | other    | same size as index
//   ---------+----------+-------------------
//   array    | anything | same size as index


I'm attaching a test script and associated function that can be used to verify these assumptions.  If correct it should produce no output.  Could someone test this in a recent Matlab and verify that it is correct?  It fails a number of cases in Octave currently.

I'm also attaching a proposed patch for Octave to fix these problems.

Note that I had to add data members to idx_vector to keep track of the real original index dimensions and whether it is a logical index.  In spite of the fact that there is an orig_dims data member there, it seems like it can be something other than the original dimensions of the index.  Also, there's a lot of complication there with transforming one type of index to another (logical to vector, for example) so I'm not sure whether my change is actually sufficient since the notion of "logical index" is not transferred if the internal index representation changes.  It would be great to simplify and clean up this mess but I can't work on it more at the moment.


(file #40367, file #40368, file #40369)

John W. Eaton <jwe>
Group administrator
Tue 11 Apr 2017 07:29:39 PM UTC, comment #1: 

The issue is not so much logical/linear indexing, rather it is the determination of what the indexed object is.

For indexing, where


S = scalar
V = vector
M = matrix

R = result



Case 1: scalar indexed with scalar, vector, matrix
S(S)
S(V)
S(M)

shape of result = shape of indexing S, V, M object.



Case 2: vector indexed with scalar, vector, matrix
V(S) = scalar (shape of indexing object)
V1(V2) = vector, BUT, retains shape of original object V1
V(M) = matrix (shape of indexing object)



Case 3: matrix indexed with scalar, vector, matrix
M(S)
M(V)
M(M)

shape of result = shape of indexing S, V, M object.


As you can see, there is only one odd case which is a vector indexed by a vector.  Octave treats the 3-D example matrix as a matrix, which means the resulting shape is the shape of the indexing object (a vector).


octave:3> x = reshape (1:3, 1,1,3)
x =

ans(:,:,1) =  1
ans(:,:,2) =  2
ans(:,:,3) =  3

octave:4> x([3 2 1])
ans =

   3   2   1

octave:5> x([3 2 1]')
ans =

   3
   2
   1


Notice how the output follows the orientation of the input as it should.  On the other hand, Matlab seems to be treating the original matrix as a vector and thus retaining the shape of the original object.  What does this return in Matlab?


x = reshape (1:3, 1,1,3);
isvector (x)


My guess is that they are internally inconsistent and they don't think 'x' is a vector, except for the odd case of indexing.



Rik <rik5>
Group administrator
Mon 10 Apr 2017 10:12:45 AM UTC, original submission:  

Octave does not use the same rules for doing logical indexing on vectors as Matlab in terms of the shape of the output. In Matlab the rule seems to be: for vectors, return always the same shape of vector being indexed, for anything else return a column vector

For example, in Matlab:


>> a = reshape (1:3, 1,1,3)

a(:,:,1) =

     1.0000e+000


a(:,:,2) =

     2.0000e+000


a(:,:,3) =

     3.0000e+000

>> b = true(1,3)

b =

  1×3 logical array

   1   1   1

>> a(b)

ans(:,:,1) =

     1.0000e+000


ans(:,:,2) =

     2.0000e+000


ans(:,:,3) =

     3.0000e+000

>> size (a(b))

ans =

     1.0000e+000     1.0000e+000     3.0000e+000


In Octave:


octave:15> a = reshape (1:3, 1,1,3)
a =

ans(:,:,1) =  1
ans(:,:,2) =  2
ans(:,:,3) =  3

octave:16> b = true(1,3)
b =

   1   1   1

octave:17> a(b)
ans =

   1   2   3

octave:18> size(a(b))
ans =

   1   3


This is using a dev version of Octave, but from quite a while ago, but I've even deleted the repo clone I built it from, so I can't give hg id:


octave:19> ver
----------------------------------------------------------------------
GNU Octave Version: 4.1.0+
GNU Octave License: GNU General Public License
Operating System: Linux 4.4.0-34-generic #53~14.04.1-Ubuntu SMP Wed Jul 27 16:56:40 UTC 2016 x86_64
----------------------------------------------------------------------
Package Name  | Version | Installation directory
--------------+---------+-----------------------
     control  |   2.8.1 | /home/rcrozier/octave/control-2.8.1
      odepkg *|   0.8.5 | /home/rcrozier/octave/odepkg-0.8.5
      signal  |   1.3.2 | /home/rcrozier/octave/signal-1.3.2




Richard <crobar>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #40367:  idxtest.m added by jwe (640B - text/x-csrc)
file #40368:  vecidxtest2.m added by jwe (7KiB - text/x-csrc)
file #40369:  idx-diffs.txt added by jwe (8KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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
    2017-04-12 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2017-04-12 jwe StatusPatch Submitted Ready For Test
    2017-04-11 jwe Attached File- Added idxtest.m, #40367
        Attached File- Added vecidxtest2.m, #40368
        Attached File- Added idx-diffs.txt, #40369
        StatusConfirmed Patch Submitted
    2017-04-11 rik5 StatusNone Confirmed
        SummaryOctave logical indexing inconsistent with Matlab Octave inconsistent with Matlab for corner indexing case
    2017-04-10 crobar Carbon-Copy- Added crobar

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code