bugGNU Octave - Bugs: bug #45171, isequal fails for large sparse...

 
 

bug #45171: isequal fails for large sparse matrices (subscript indixes too large)

Submitter:  Marco Caliari <caliari>
Submitted:  Sat 23 May 2015 11:26:25 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  caliari
Originator Name:  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

Fri 23 Mar 2018 09:45:33 PM UTC, comment #10: 

@Mike: If you notice something else, let me know.

Kai Torben Ohlhus <siko1056>
Group Member
Fri 23 Mar 2018 09:25:33 PM UTC, comment #9: 

So sorry for the noise, I am testing too many things at once. I must have been accidentally running the new tests but with the old isequal.m function in the load path. The tests do pass. Closing again.

Mike Miller <mtmiller>
Group Member
Fri 23 Mar 2018 09:13:48 PM UTC, comment #8: 

Kai - this change (0e5ae3ecda1a) on stable introduces 6 new tests, all of which fail on Octave built for 32-bit i386 with an "out of memory or dimension too large" error. Can you take a look at that?

Mike Miller <mtmiller>
Group Member
Thu 22 Mar 2018 03:17:45 PM UTC, comment #7: 

This is still a problem in the current stable.  I updated the original patch file #34084 to meet the current implementation and pushed it: http://hg.savannah.gnu.org/hgweb/octave/rev/0e5ae3ecda1a

Kai Torben Ohlhus <siko1056>
Group Member
Fri 18 Nov 2016 07:15:18 PM UTC, comment #6: 

I would venture to say that numel() is preferred over length() in the Octave code base these days even when it is guaranteed that the variable references a vector.

Mike Miller <mtmiller>
Group Member
Thu 17 Nov 2016 08:38:43 AM UTC, comment #5: 

Hi, now that 4.2 is out I'd like to apply this change.
Can it be pushed as is or should I replace length by numel?

Carlo de Falco <cdf>
Group Member
Wed 28 Sep 2016 02:54:51 PM UTC, comment #4: 

x is the result of


[i_x,j_x,x] = find (x);


so even if the original x was a matrix after this call
it will be a vector.

similarly for y below in the code

nonetheless I see no problem in using numel instead of length if you prefer ...

Carlo de Falco <cdf>
Group Member
Wed 28 Sep 2016 02:49:52 PM UTC, comment #3: 

I agree that in the original code, length is OK because it is applied to the result of the find function.  But in the modified version it is applied to X.  Can that be a matrix?

John W. Eaton <jwe>
Group administrator
Wed 28 Sep 2016 11:05:32 AM UTC, comment #2: 

This patch was being discussed on IRC yesterday,
when my connection went suddenly off, I'll
continue the discussion here.

Just before communication was interrupted jwe asked:


<jwe> cdf: in that last one, is length the right thing to apply to x?  That's a strange function because it returns the larger of the number of rows or columns.


My answer would have been:

I would say length is equivalent to numel in this context because find should always return vectors, as noted in its
documentation.

But if you prefer it I see no problem in replacing length by numel in the patch.

Also note that the use of length was there before this change.


Carlo de Falco <cdf>
Group Member
Thu 22 Sep 2016 09:04:36 AM UTC, comment #1: 

This report is quite old, but by browsing for "__isequal__" it got my attention. Your problem does not exist, if you are using 64-bit indices, as you are touching the critical border:


>> [46342^2; 2^31]
ans =

   2147580964
   2147483648


What prevents you from applying your patch before the 4.2 release?

Kai Torben Ohlhus <siko1056>
Group Member
Sat 23 May 2015 11:26:25 AM UTC, original submission:  

Dear all,

the following code


A=speye(46342);,isequal(A,A)


fails with


error: __isequal__: subscript indices must be either positive integers less than 2^31 or logicals


It is the already seen problem of linear indexing of large sparse matrices. On the other hand, I was able to fix this case with the enclosed patch, still preserving the 26 tests in isequal.m.

Cheers,

Marco

Marco Caliari <caliari>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #34084:  __isequal__.m.patch added by caliari (673B - 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 mtmiller (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by cdf (Posted a comment)
  • -email is unavailable- added by cdf
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by caliari (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 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-03-23 mtmiller StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2018-03-23 mtmiller StatusFixed In Progress
        Open/ClosedClosed Open
    2018-03-22 siko1056 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
        Release4.2.0 dev
    2016-11-17 mtmiller Release4.2.0-rc2 4.2.0
    2016-09-28 cdf Carbon-Copy- Added -email is unavailable-
    2016-09-22 siko1056 Item GroupNone Incorrect Result
        StatusNone Patch Submitted
        Assigned toNone caliari
        Release3.8.2 4.2.0-rc2
    2015-05-23 caliari Summaryisequal fails for large sparse matrices (subscript indices too large) isequal fails for large sparse matrices (subscript indixes too large)
    2015-05-23 caliari Attached File- Added _isequal_.m.patch, #34084

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code