bugGNU Octave - Bugs: bug #62277, QR needs to accept...

 
 

bug #62277: QR needs to accept "econ" argument for Matlab compatibility

Submitter:  Arun Giridhar <arungiridhar>
Submitted:  Sat 09 Apr 2022 11:30:46 AM UTC
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

Sat 09 Apr 2022 09:54:49 PM UTC, comment #10: 

@arun: I added a commit message and checked in the patch under your name here: http://hg.savannah.gnu.org/hgweb/octave/rev/7ad60a258a2b.

Marking as fixed and closing report.

Rik <rik5>
Project Administrator
Sat 09 Apr 2022 07:58:17 PM UTC, comment #9: 

Patch 3 attached. Edited the error message and added a couple more BISTs. Now it says this:

octave:1> [q,r] = qr (magic(3), rand(3,1), "dfghlkjhdfg")
error: qr: option string must be 'econ' or 'matrix' or 'vector', not "dfghlkjhdfg"

(file #53061)

Arun Giridhar <arungiridhar>
Project Member
Sat 09 Apr 2022 03:58:19 PM UTC, comment #8: 

I reviewed both the functional patch and the documentation change and it looks good to me.  Marking as "Patch Reviewed".

One thing I would change is to remove the @qcode macro from the word "economy".  The @qcode macro is to be used for actual code that a user would type.  But it is impossible to call

qr (..., "economy")

The @qcode is used several times incorrectly around the word "economy".

This has nothing to do with the current patch, but I found it while reviewing.  The following code with an error produces a message which would be easier to understand if the malformed argument was in quotes.

qr (magic(3), 'econonomy')
error: qr: type for P must be 'matrix' or 'vector', not econonomy

I think this would be easier if the message said

error: qr: type for P must be 'matrix' or 'vector', not "econonomy"

It should be simple to change the error message format string to add quotes around the bad argument.

Rik <rik5>
Project Administrator
Sat 09 Apr 2022 12:59:56 PM UTC, comment #7: 

That's perfectly fine -- thank you for clarifying the difference. I am attaching an updated patch that updates the docstring.

(file #53058)

Arun Giridhar <arungiridhar>
Project Member
Sat 09 Apr 2022 12:57:04 PM UTC, comment #6: 

I'm marking this as a feature request because the "econ" option is a new feature.  I'd like to reserve the "Matlab Compatibility" tag for those cases in which Octave already provides the same syntax or option but has a different though still reasonable behavior.  For other things, it's either a feature request or some other kind of error.

John W. Eaton <jwe>
Project Administrator
Sat 09 Apr 2022 12:50:11 PM UTC, comment #5: 

I haven't tested your changes yet. But that flag should also be properly documented in the docstring.

Markus Mützel <mmuetzel>
Project Member
Sat 09 Apr 2022 12:42:29 PM UTC, comment #4: 

@mmuetzel: we had a simulpost and crossed. Yes, that is what I was referring to, but it seems Matlab takes an "econ" input argument (https://www.mathworks.com/help/matlab/ref/qr.html) and Octave doesn't, so my change is not new functionality but just a compatibility fix.

I will edit the wiki to remove that from the list of projects.

Arun Giridhar <arungiridhar>
Project Member
Sat 09 Apr 2022 12:38:17 PM UTC, comment #3: 

From the documentation of `qr`:

> If the final argument is the scalar 0 an "economy" factorization is returned. If the original matrix A has size MxN and M > N, then the "economy" factorization will calculate just N rows in R and N columns in Q and omit the zeros in R. If M ≤ N, there is no difference between the economy and standard factorizations. When calculating an "economy" factorization and A is dense, the output P is always a vector rather than a matrix. If A is sparse, output P is a sparse permutation matrix.
> Background: The QR factorization has applications in the solution of least squares problems
> min norm (A*x - b)
> for overdetermined systems of equations (i.e., A is a tall, thin matrix).
> The permuted QR factorization [Q, R, P] = qr (A) allows the construction of an orthogonal basis of span (A).


Is that what you were referring to?

Markus Mützel <mmuetzel>
Project Member
Sat 09 Apr 2022 12:38:14 PM UTC, comment #2: 

The attached change works for me and passes new BISTs. Please review. If it works I'll add a commit message and push.

(file #53057)

Arun Giridhar <arungiridhar>
Project Member
Sat 09 Apr 2022 11:50:22 AM UTC, comment #1: 

Looking at the qr code https://hg.savannah.gnu.org/hgweb/octave/file/cd38f31e9b4b/libinterp/corefcn/qr.cc, it looks like there is an economy mode already but it only doesn't accept the "econ" keyword. Is that correct?

Could someone please review and confirm?

If we have this functionality I'll remove it from the wiki and tell the volunteer on Discourse not to reimplement it.

Arun Giridhar <arungiridhar>
Project Member
Sat 09 Apr 2022 11:30:46 AM UTC, original submission:  

Recent Matlab versions accept the "econ" input keyword for the qr function that will allow for the reduced QR decomposition for tall and narrow input matrices. Octave currently errors on the econ keyword.

Someone expressed interest in implementing this at https://octave.discourse.group/t/octave-project-idea-more-memory-efficient-qr/2507/2

The memory-efficient reduced QR was already listed as a project at https://wiki.octave.org/Projects

What's a good way to proceed for compatibility when the functionality isn't there yet? Accept "econ" and treat it as full QR?

Arun Giridhar <arungiridhar>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #53061:  qr_econ3.patch added by arungiridhar (4KiB - text/x-patch)
file #53058:  qr_econ2.patch added by arungiridhar (4KiB - text/x-patch)
file #53057:  qr_econ.patch added by arungiridhar (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by arungiridhar (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 project members can vote.

     

     

     

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-04-09 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2022-04-09 arungiridhar Attached File- Added qr_econ3.patch, #53061
    2022-04-09 rik5 StatusPatch Submitted Patch Reviewed
    2022-04-09 jwe CategoryNone Octave Function
    2022-04-09 arungiridhar Attached File- Added qr_econ2.patch, #53058
    2022-04-09 jwe Item GroupMatlab Compatibility Feature Request
    2022-04-09 arungiridhar CategoryOctave Function None
        StatusIn Progress Patch Submitted
    2022-04-09 mmuetzel CategoryNone Octave Function
        StatusNone In Progress
    2022-04-09 arungiridhar Attached File- Added qr_econ.patch, #53057

    Back to the top


    Powered by Savane 3.9