bugGNU Octave - Bugs: bug #65544, cross.m: vector dimension should...

 
 

bug #65544: cross.m: vector dimension should produce error, can produce erroneous result

Submitter:  Nicholas Jankowski <nrjank>
Submitted:  Sun 31 Mar 2024 11:32:26 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 9.1.0 Operating System:  * Any
Fixed Release:  None Planned Release:  9.2.0 (current stable)
* Mandatory Fields

Add a New Comment Rich Markup
   

Sat 06 Apr 2024 12:32:27 AM UTC, comment #5: 

Nah, it's fine.  Interesting that there is a performance difference.  I suppose isrow() also has to check "ndims (x) == 2" and that adds something.

Marking as Fixed and closing report.

Rik <rik5>
Group administrator
Thu 04 Apr 2024 09:17:50 PM UTC, comment #4: 

it originally was rows(x)==1, which would be a problem if the ndims(x)<3 constraint wasn't there. As i was testing code snippets related to changing the output orientation I somehow missed that constraint, so my first push switched to isrows/iscolumns to prevent it from passing dim>2 to the transpose ops (even though it can't happen). there's a slight performance hit with isrows vs rows(x)==1, so i switched it back. with the ndims check they're equivalent in function, so it would be fine either way if you'd prefer it changed back for clarity.


Nicholas Jankowski <nrjank>
Group Member
Thu 04 Apr 2024 06:50:04 PM UTC, comment #3: 

Question on coding style, in the very latest cset of 4/1 there is


--- a/scripts/linear-algebra/cross.m
+++ b/scripts/linear-algebra/cross.m
@@ -67,14 +67,14 @@

   nd = ndims (x);

-  if (nd < 3 && ndims (y) < 3 && nargin < 3)
+  if (nargin < 3 && nd < 3 && ndims (y) < 3)
     ## COMPATIBILITY -- opposite behavior for cross(row,col)
     ## Swap x and y in the assignments below to get the matlab behavior.
     ## Better yet, fix the calling code so that it uses conformant vectors.
-    if (iscolumn (x) && isrow (y))
+    if (columns (x) == 1 && rows (y) == 1)
       warning ("cross: taking cross product of column by row");
       y = y.';
-    elseif (isrow (x) && iscolumn (y))
+    elseif (rows (x) == 1 && columns (y) == 1)
       warning ("cross: taking cross product of row by column");
       x = x.';
     endif


Why is it better to use "rows (x) == 1" rather than "isrow (x)"?  I think isrow () is clearer, but maybe I'm missing something.  This section is already constrained only to operate when the objects are 2-D.

Rik <rik5>
Group administrator
Mon 01 Apr 2024 05:06:07 PM UTC, comment #2: 

minor update pushed to
https://hg.savannah.gnu.org/hgweb/octave/rev/2aea823a9973
(just noticed bug number typo in commit msg)

changed back the vector mismatch check as i misinterpreted what would sneak through the original check, and the performance is slightly better with the previous version.  Also adjusted the dim 3 element check so it would hit the same error message whether x or y was sized.

comment on bug #65527 indicated the input validation changes perhaps should go on default. In this case, the original cases were either inconsistently triggering lowel level errors with less meaningful messages or were silently producing incorrect results. As such I felt stable was appropriate.  If this should be moved to default instead i can back it out and move it.

Nicholas Jankowski <nrjank>
Group Member
Mon 01 Apr 2024 02:18:55 PM UTC, comment #1: 

pushed improved input validation patch to stable as:
https://hg.savannah.gnu.org/hgweb/octave/rev/308060bc6b27

also fixes the case where a 1xnxm or nx1xm arrays would get passed to the transpose operator meant for row/column vector operations.  Added input validation BISTs for all error messages/warnings and removed the FIXME note.

marking as ready for test.

Nicholas Jankowski <nrjank>
Group Member
Sun 31 Mar 2024 11:32:26 PM UTC, original submission:  

following on the discussion on bug #65527 with compiled functions not catching non-scalar dims, realized that cross does the same but instead of just using dim(1) and ignoring the rest, it can either produce not-overly-meaningful errors or silently produce incorrect output:


a = magic(3);
b = eye(3);

cross(a,b)
ans =

   0  -9   7
   4   0  -6
  -3   1   0

cross(a,b,1)
ans =

   0  -9   7
   4   0  -6
  -3   1   0

cross(a,b,2)
ans =

   0   6  -1
  -7   0   3
   9  -4   0

cross(a,b,[1 2])
ans =

   3
  -6
   3

cross(a,b,[2 1])
ans =

   3  -6   3

cross(a,b,[3 1])
error: x(_,_,2): out of bound 1 (dimensions are 3x3)
error: called from
    cross at line 96 column 8


there is no vecdim compatibility concern here, even matlab requires it to be an integer valued positive scalar.

also looks like there's a FIXME to add a lot more input validation.  working up a patch now that will add an error if dim doesn't meet the conditions above, and add BISTS against stable

Nicholas Jankowski <nrjank>
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

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by nrjank (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-04-06 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2024-04-01 nrjank StatusNone Ready For Test

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code