bugGNU Octave - Bugs: bug #32966, spdiags is slow and buggy

 
 

bug #32966: spdiags is slow and buggy

Submitted by:  Marco Caliari <caliari>
Submitted on:  Thu 31 Mar 2011 01:21:38 PM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: GNU/Linux

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

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

 

(Jump to the original submission Jump to the original submission)

Thu 19 Dec 2013 12:16:20 AM UTC, comment #6:

Marco,

I rewrote spdiags extraction routine to use diag() in C++. It is about 20% faster than the previous code. I also overhauled the function (http://hg.savannah.gnu.org/hgweb/octave/rev/190ef1764d30). Closing report.

Rik <rik5>
Project Administrator
Wed 18 Dec 2013 04:21:23 PM UTC, comment #5:

Marco,

I applied one of your patches that changes diag() to return an empty matrix rather than issue a warning when the requested diagonal is out of range (http://hg.savannah.gnu.org/hgweb/octave/rev/a86d608c413c). I didn't apply the second patch which speeds up spdiags by using diag() internally because it fails on the first example given in the Matlab documentation (http://www.mathworks.com/help/matlab/ref/spdiags.html).

--Rik

Rik <rik5>
Project Administrator
Thu 14 Mar 2013 07:08:21 AM UTC, comment #4:

I'm sorry Rik, but I won't be able to contribute to Octave for the next few months. I'm looking forward to start again.

Thanks,

Marco

Marco Caliari <caliari>
Project Member
Tue 12 Mar 2013 11:19:16 PM UTC, comment #3:

Marco, this bug report is 1.5 years old now. Do you want to rebase your patches onto a recent Mercurial changeset and close this report?

Rik <rik5>
Project Administrator
Fri 30 Sep 2011 04:06:19 PM UTC, comment #2:

Marco,

This wasn't the case when you filed this bug report, but you are part of the Octave team now and have commit privileges to Mercurial. Do you want to just apply the patches in this bug report yourself and close it? The performance gains seem impressive.

Rik <rik5>
Project Administrator
Fri 08 Apr 2011 01:54:26 PM UTC, comment #1:

I'm submitting two patches: diag.patch fixes the compatibility issue 3). spdiags.m.patch rewrites the function calls spdiags (A) and spdiags(A, C) and uses diag to extract diagonals. This fixes issues 1) and 2). Infact, using the testfile testspdiags.m (which creates sparse matrices of random sizes and extract random diagonals), I see

+verbatim-

> testspdiags(10) % random sparse matrices up to size 10

warning: spdiagsorig failed
...
warning: spdiagsorig failed
cpuorigtot = 10.013
cputot = 0.85205
-verbatim-

Because of issue 2) in the original spdiags, sometimes you get the warning. Two compatibility issues remain: number 4) (which I think is a bug in ml). Then, if A is a vector, ml gives an error, whereas the original spdiags in Octave (and mine) works. When you call spdiags with 1 or 2 input args you want to extract diagonals (no ambiguity as in diag): so a vector is just a rectangular matrix.

Marco

(file #23138, file #23139, file #23140)

Marco Caliari <caliari>
Project Member
Thu 31 Mar 2011 01:21:38 PM UTC, original submission:

Dear maintainers,

stimulated by the comment
in spdiags.m, I found the following:

1) diag is much faster then spdiags in extracting diagonals. I get

2) spdiags fails in the following case:
whereas it works with [1,1;1,1].

3) diag is not compatible:
whereas Matlab gives empty matrix 0-by-1.

If 3) is fixed, I can easily rewrite spdiags.m to avoid problems 2) and 1) (without testing if the diagonal is out of range). However, I'm not clever and I cannot do it without a loop :-(

4) there is another incompatible behaviour:
whereas in Matlab
I do not understand this Matlab (7.6.0) behaviour.

If we take a decision about 4) and someone fixes 3), I can work on 1) and 2).

Cheers,

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 #23138:  diag.patch added by caliari (1kB - text/x-patch)
file #23139:  spdiags.m.patch added by caliari (2kB - text/x-patch)
file #23140:  testspdiags.m added by caliari (612B - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mtmiller (Updated the item)
  • -unavailable- added by rik5 (Updated the item)
  • -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 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 19 Dec 2013 12:16:20 AM UTCrik5StatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Wed 18 Dec 2013 04:21:23 PM UTCrik5Summaryspdiags is slow and buggy, diag is not compatible=>spdiags is slow and buggy
    Mon 28 Oct 2013 02:21:36 AM UTCmtmillerCategoryNone=>Octave Function
      Release3.4.0=>dev
    Thu 04 Aug 2011 05:51:13 AM UTCrik5StatusNone=>Patch Submitted
    Fri 08 Apr 2011 01:54:26 PM UTCcaliariAttached File-=>Added diag.patch, #23138
      Attached File-=>Added spdiags.m.patch, #23139
      Attached File-=>Added testspdiags.m, #23140

    Back to the top


    Powered by Savane 3.1-cleanup