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

 
 

bug #46912: qr not working as described

Submitted by:  Marco Caliari <caliari>
Submitted on:  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 Andrew <lachlan>
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  GNU/Linux

Add a New Comment (Rich Markup)
   

You are not logged in

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

 

( 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>
Project 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>
Project 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>In charge of this item.
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>In charge of this item.
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>In charge of this item.
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>In charge of this item.
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>
Project 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>In charge of this item.
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>
Project 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>
Project Administrator
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>
Project 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>In charge of this item.
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>
Project 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>In charge of this item.
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>
Project 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)
  •  

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

    Only project 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.5