bugGNU Octave - Bugs: bug #46912, qr not working as described

 
 

bug #46912: qr not working as described

Submitter:  Marco Caliari <caliari>
Submitted:  Sat 16 Jan 2016 08:09:04 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  lachlan
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 05 Aug 2016 07:19:46 PM UTC, comment #14: 

I made one more fix to use @dots macro http://hg.savannah.gnu.org/hgweb/octave/rev/cfa684a0539d

Carnë Draug <carandraug>
Group Member
Fri 05 Aug 2016 06:36:56 PM UTC, comment #13: 

I checked in the changeset from file #37746 here:

http://hg.savannah.gnu.org/hgweb/octave/rev/99454a60bf5e

Can this report be closed?

John W. Eaton <jwe>
Group administrator
Thu 07 Jul 2016 08:56:21 AM UTC, comment #12: 

The attached patch reflects the change of format of docstrings.

Marco, it should now be able to be applied after file #36374 for bug #41567.

(file #37746)

Lachlan Andrew <lachlan>
Thu 18 Feb 2016 05:02:48 AM UTC, comment #11: 

Here's a fresh patch to reflect jwe's recent changes.

(file #36373)

Lachlan Andrew <lachlan>
Sat 30 Jan 2016 12:41:53 AM UTC, comment #10: 

Oops.  The previous patch had some debugging output, and was lax with error checking.  The v3 patch should be OK (I hope :)



(file #36203)

Lachlan Andrew <lachlan>
Sat 23 Jan 2016 05:38:01 AM UTC, comment #9: 

Thanks jwe, you're absolutely right.  (I was somehow confusing "qr(A,'matrix')" and "qr(A,B)".)

I've written a revised patch, which assumes the patch for bug #41567 has been applied.

It also implements the 'matrix' and 'vector' options for full matrices.  Sparse matrices don't seem to support a third output argument; the patch explicitly throws an error in that case.

(file #36137)

Lachlan Andrew <lachlan>
Wed 20 Jan 2016 05:01:42 PM UTC, comment #8: 

It looks to me like


[q, r, p] = qr (a)


is equivalent to


[q, r, p] = qr (a, 'matrix')


and that


[q, r, p] = qr (a, 'vector')


just returns p in a vector form such that instead of writing


a * p == q * r


you can write


a(:,p) == q * r


So I don't think that any knowledge of the Fortran libraries is really needed to implement these features.  It's just a matter of accepting 'matrix' as the default and, for the 'vector' argument, transforming the permutation matrix to a vector of indices.  I believe that's equivalent to writing


find (p) - (0:rows(p):numel(p)-1)'


though I'm sure there's a better way of expressing that in C++ when using the permutation matrix class.  I think it is just the col_perm_vec method in the PermMatrix class.

John W. Eaton <jwe>
Group administrator
Wed 20 Jan 2016 04:29:00 PM UTC, comment #7: 

Thanks for the feedback.  I'm not (yet?) sufficiently familiar with the Fortran libraries to extend the functionality to qr (A, 'matrix'), which is why the patch I submitted is just to get the current functionality correct.  I'll look at the patch for bug #41567.

I can fix point 4.

For point 3, do you mean that "vector" is the same as not having vector, except that P is a permutation vector instead of a permutation matrix?  (I'll read Matlab's help before doing anything...)  If so, I can fix that too.

Lachlan Andrew <lachlan>
Tue 19 Jan 2016 07:05:09 PM UTC, comment #6: 

Mike, you are right, sorry for the noise.

Of course #41567 is related and the correct reference help is

http://www.mathworks.com/help/matlab/ref/qr.html

W.r.t. the points in comment #4:

1) the first part is still valid
2) not valid
3) and 4) valid
5) not valid

W.r.t. comment #2 in #41567, qr(A,B) throws an error in ML if A is full.

Marco Caliari <caliari>
Group Member
Tue 19 Jan 2016 05:30:12 PM UTC, comment #5: 

It seems that you're reading from the symbolic version of QR now, maybe compare these two help references:

http://www.mathworks.com/help/matlab/ref/qr.html
http://www.mathworks.com/help/symbolic/qr.html

The syntaxes are slightly different for each, the latter operates on @sym data.

Also, does bug #41567 have any relevance here? Can one of you please review the discussion and patch submitted on that bug report? Maybe it is a subset of the work being done here?

Mike Miller <mtmiller>
Group Member
Tue 19 Jan 2016 08:59:26 AM UTC, comment #4: 

Thanks,

I did not try, but from your patch it seems to me:

1) qr(A,'matrix'), A full, throws an error. This is not compatible with Matlab. In fact, [Q,R]=qr(A,'matrix') just works in Matlab and [Q,R,P]=qr(A,'matrix') returns P as a permutation matrix (default behavior). UPDATE: this is true for ML2014b, but it seems that ML2015b changed a little bit

http://it.mathworks.com/help/symbolic/qr.html

Can anyone test [Q,R]=qr(rand(4),'matrix') and [Q,R,P]=qr(rand(4),'matrix') with ML2015b?

2) ML2015b changed other things: the option '0' seems to be not supported anymore and replaced with 'econ'. Can anyone test [Q,R]=qr(rand(4),0) with ML2015b?

3) the option 'vector' is not supported in Octave. It is similar to '0', but zero rows of R are not omitted. For instance


A=reshape(1:6,3,2)
[Q,R,P]=qr(A,'vector')
Q =

   -0.4558    0.7909    0.4082
   -0.5698    0.0930   -0.8165
   -0.6838   -0.6048    0.4082


R =

   -8.7750   -3.6467
         0   -0.8374
         0         0


P =

     2     1


4) In the description of [Q,R,P]=qr(A) I would add that Q*R=A*P

5) The new option 'real' in ML2015b is not supported.

Sorry that I did not report all the issues in the original submission.

Marco Caliari <caliari>
Group Member
Tue 19 Jan 2016 05:55:58 AM UTC, comment #3: 

Marco, you're right that the issue with qr(A,B) is just the documentation.  However, there was a bug for qr (A, B, complex(0)).  Both are fixed by the attached changeset.

The other issue with P being a vector is because qr(A,anything) is interpreted as being qr(A,0), which causes the "economy" output.  The attached changeset causes it to throw an error for qr(A,anything-except-zero), unless A is sparse.

Please reply saying if it now gives the behaviour you expect.



(file #36090)

Lachlan Andrew <lachlan>
Mon 18 Jan 2016 10:27:37 AM UTC, comment #2: 

Looking at the code, I now understand that it is just a problem in the description. In fact, qr(A,B) and qr(A,B,'0') are valid only for a sparse matrix A. This should be more clear in the help and informative errors messages should be given otherwise.
Then, I see that [Q,R,P] = qr(A,anything), for a full matrix A, gives P as a vector. This is not compatible with qr(A,'matrix') in Matlab.

Marco Caliari <caliari>
Group Member
Mon 18 Jan 2016 05:09:19 AM UTC, comment #1: 

Thanks for the bug report Marco.  I'll get back to you soon.

Lachlan Andrew <lachlan>
Sat 16 Jan 2016 08:09:04 AM UTC, original submission:  

There are several problems with qr. First of all, the most common use [Q, R] = qr (A) is not described in the initial sequence of valid calls. Then, in the description of the call [Q, R, P] = qr (A) I think it would be useful to write that Q*R=A*P. Then, [C, R] = qr (A, B) simply ignores B and returns C*R=A. Finally, [C, R] = qr (A, B, '0') gives an error.

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 #37746:  bug_46912.v5.cset added by lachlan (12KiB - text/x-diff)
file #36373:  bug_46912.v4.cset added by lachlan (13KiB - application/octet-stream)
file #36203:  bug_46912.v3.cset added by lachlan (12KiB - application/octet-stream)
file #36137:  bug_46912.v2.cset added by lachlan (12KiB - application/octet-stream)
file #36090:  bug_46912.cset added by lachlan (6KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by lachlan (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-08 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2016-08-05 jwe StatusPatch Submitted Ready For Test
    2016-07-07 lachlan Attached File- Added bug_46912.v5.cset, #37746
        Release4.0.0 dev
    2016-02-18 lachlan Attached File- Added bug_46912.v4.cset, #36373
        Assigned toNone lachlan
    2016-01-30 lachlan Attached File- Added bug_46912.v3.cset, #36203
    2016-01-23 lachlan Attached File- Added bug_46912.v2.cset, #36137
    2016-01-19 mtmiller StatusNone Patch Submitted
    2016-01-19 lachlan Attached File- Added bug_46912.cset, #36090

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code