bugGNU Octave - Bugs: bug #35570, Indexing of sparse vectors

 
 

bug #35570: Indexing of sparse vectors

Submitted by:  Marco Caliari <caliari>
Submitted on:  Mon 20 Feb 2012 03:43:16 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalItem Group: None
Status: FixedAssigned to: Jordi Gutiérrez Hermoso <jordigh>
Originator Name: Open/Closed: Closed
Release: 3.6.1Operating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Mon 05 Mar 2012 02:42:44 PM UTC, comment #5:

Thanks, I pushed a modified version of your patch:

http://hg.savannah.gnu.org/hgweb/octave/rev/ec660526ae50

Since this was a regression, I patched on the stable branch.

Note the following modifications over your patch:

1) The commit message is different. Short one-line description, longer description saying the file and function in which the change is.

2) Added a test to verify the correctness of this change.

3) No tabs! For some reason, your second diff still had tabs.

4) Spaces before opening parentheses in function calls, but not in front of indexing. You were missing a couple of spaces.

5) And it's done with hg. ;-)

If you submit future patches, please try to follow these suggestions.

Thanks again, closing this report.

Jordi Gutiérrez Hermoso <jordigh>
Project AdministratorIn charge of this item.
Fri 02 Mar 2012 08:00:37 AM UTC, comment #4:

Dear Jordi,

please find enclosed the new patch. I'm sorry that I'm not ready for a hg changeset yet.

Marco

2012-02-03 Marco Caliari (marco.caliari@univr.it)

* Sparse.cc: fix indexing of a sparse column vector when idx is a range with increment -1. Bug # 35570.

(file #25237)

Marco Caliari <caliari>
Project Member
Thu 01 Mar 2012 04:25:23 PM UTC, comment #3:

I think Marco's patch is essentially correct and I have no idea how whoever wrote the code that Marco is correcting could have been correct. I always try to be charitable towards code already in place, and I can't see how anyone thought the code in place could have been correct.

Basically, in this code path:

http://hg.savannah.gnu.org/hgweb/octave/file/c267a3522d6c/liboctave/Sparse.cc#l1402

what should happen is that the return matrix should have the data vector reversed (that part is correct) the row vector reversed but all the indices have to be recomputed, and the column vector should be copied. These last two things are completely wrong.

I'm not even sure why this code path exists, though. As far as I can tell, it's already handled by other paths, and looks like an unnecessarily aggressive and completely flawed optimisation.

Marco, if you could resubmit your patch without tabs, with a commit message, and as an hg changeset, that would be grand. Otherwise, I'll do it myself later.

Jordi Gutiérrez Hermoso <jordigh>
Project AdministratorIn charge of this item.
Wed 22 Feb 2012 04:38:35 PM UTC, comment #2:

It seems that the enclosed patch for 3.6.0 solves the problem. But somebody really understanding that piece of code should review it before applying.

Marco

(file #25114)

Marco Caliari <caliari>
Project Member
Mon 20 Feb 2012 08:05:52 PM UTC, comment #1:

The bug is even a bit odder than it first seems. If the range is not exactly over the entire column then the range index works correctly. Also, if the matrix has more than one column the indexing works.

Examples:

Rik <rik5>
Project Administrator
Mon 20 Feb 2012 03:43:16 PM UTC, original submission:

Dear maintainers,

the following is right

while the following is wrong

The bug is present since 3.4.0 and not in 3.2.4.

Best regards,

Marco

Marco Caliari <caliari>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #25237:  Sparse.cc.diff added by caliari (705B - text/x-patch)
file #25114:  Sparse.cc.diff added by caliari (587B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jordigh (Updated the item)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by caliari (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 05 Mar 2012 03:07:43 PM UTCjordighAssigned toNone=>jordigh
    Mon 05 Mar 2012 02:42:44 PM UTCjordighStatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
      Release3.6.0=>3.6.1
    Fri 02 Mar 2012 08:00:37 AM UTCcaliariAttached File-=>Added Sparse.cc.diff, #25237
    Sat 25 Feb 2012 07:50:31 PM UTCjordighStatusConfirmed=>Patch Submitted
    Wed 22 Feb 2012 04:38:35 PM UTCcaliariAttached File-=>Added Sparse.cc.diff, #25114
    Mon 20 Feb 2012 11:51:33 PM UTCrik5StatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1