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 FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: Lachlan Andrew <lachlan>
Originator Name: Open/Closed: Closed
Release: devOperating 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 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>
Project MemberIn 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>
Project MemberIn 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>
Project MemberIn 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>
Project MemberIn charge of this item.
Wed 20 Jan 2016 05:01:42 PM UTC, comment #8:

It looks to me like

is equivalent to

and that

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

you can write

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

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>
Project MemberIn 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

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

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>
Project MemberIn 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>
Project MemberIn 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 QR=AP. 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 File(s):
   
   
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
  • -unavailable- added by rik5 (Updated the item)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by lachlan (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 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 08 Aug 2016 09:23:47 PM UTCrik5StatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Fri 05 Aug 2016 06:36:56 PM UTCjweStatusPatch Submitted=>Ready For Test
    Thu 07 Jul 2016 08:56:21 AM UTClachlanAttached File-=>Added bug_46912.v5.cset, #37746
      Release4.0.0=>dev
    Thu 18 Feb 2016 05:02:48 AM UTClachlanAttached File-=>Added bug_46912.v4.cset, #36373
      Assigned toNone=>lachlan
    Sat 30 Jan 2016 12:41:53 AM UTClachlanAttached File-=>Added bug_46912.v3.cset, #36203
    Sat 23 Jan 2016 05:38:01 AM UTClachlanAttached File-=>Added bug_46912.v2.cset, #36137
    Tue 19 Jan 2016 05:30:12 PM UTCmtmillerStatusNone=>Patch Submitted
    Tue 19 Jan 2016 05:55:58 AM UTClachlanAttached File-=>Added bug_46912.cset, #36090

    Back to the top


    Powered by Savane 3.1-cleanup1