Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and upgrade double dense matrix QR decomposition #10795

Closed
rbeezer mannequin opened this issue Feb 17, 2011 · 36 comments
Closed

Fix and upgrade double dense matrix QR decomposition #10795

rbeezer mannequin opened this issue Feb 17, 2011 · 36 comments

Comments

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Feb 17, 2011

The Q matrix of a QR decomposition should be unitary, hence invertible. For zero-column trivial cases, this is broken.

sage: A = zero_matrix(CDF, 5, 0)
sage: Q, R = A.QR()
sage: Q
[0 0 0 0 0]
[0 0 0 0 0]
[0 0 0 0 0]
[0 0 0 0 0]
[0 0 0 0 0]

Besides a bugfix this patch will upgrade the documentation to make it clear how this routine works over the complex numbers. In particular, SciPy routines are using a Hermitian inner product - documentation upgrade will reflect that.

Apply:

  1. attachment: trac_10795-QR-decomposition-double-dense-v2.patch
  2. attachment: trac_10795-QR-decomposition-formatting.patch

CC: @jasongrout @dandrake

Component: linear algebra

Keywords: sd40.5

Author: Rob Beezer

Reviewer: Martin Raum, Dan Drake

Merged: sage-5.2.beta1

Issue created by migration from https://trac.sagemath.org/ticket/10795

@rbeezer rbeezer mannequin added this to the sage-4.7.1 milestone Feb 17, 2011
@rbeezer rbeezer mannequin assigned jasongrout and williamstein Feb 17, 2011
@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Feb 17, 2011

Author: Rob Beezer

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Feb 17, 2011

comment:1

Just one change to the code, to return an identity matrix in a trivial case. Everything else is documentation. Unfortunately, this file is not included in the documentation (yet). You can check that it builds without warnings, then look at it in the notebook to verify the contents and appearance.

@rbeezer rbeezer mannequin added the s: needs review label Feb 17, 2011
@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented Aug 6, 2011

Reviewer: Martin Raum

@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented Aug 6, 2011

comment:2

Everything is OK with this patch.

@jdemeyer
Copy link

Merged: sage-4.7.2.alpha2

@jdemeyer
Copy link

Changed merged from sage-4.7.2.alpha2 to none

@jdemeyer
Copy link

comment:5

On various systems, there are doctest failures:

hawk (OpenSolaris 06/2009 i86pc Xeon W3580):

**********************************************************************
File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 1766:
    sage: Q
Expected:
    [ -0.359210604054   0.569326179705   0.368048420509   0.641385845805]
    [  0.179605302027  -0.144590775798   0.925041158846  -0.301884576418]
    [  0.179605302027  -0.704880032016  0.0774617736597   0.681825307224]
    [  0.898026510134   0.397624633445 -0.0532812182975   0.180566192161]
Got:
    [-0.359210604054  0.569326179705  0.409076682956  0.616028985113]
    [ 0.179605302027 -0.144590775798 -0.683704756325   0.69237507842]
    [ 0.179605302027 -0.704880032016  0.575201135981  0.374205463771]
    [ 0.898026510134  0.397624633445  0.185331397251 0.0330954856071]
**********************************************************************
File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 1771:
    sage: R
Expected:
    [    -5.56776436283       2.6940795304      -2.6940795304]
    [                 0     -3.56958477752      3.56958477752]
    [                 0                  0 -9.93013661299e-16]
    [                 0                  0                  0]
Got:
    [   -5.56776436283      2.6940795304     -2.6940795304]
    [                0    -3.56958477752     3.56958477752]
    [                0                 0 -4.4408920985e-16]
    [                0                 0                 0]
**********************************************************************

bsd (OS X 10.8.0 x86_64) 64-bit:

**********************************************************************
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 1766:
    sage: Q
Expected:
    [ -0.359210604054   0.569326179705   0.368048420509   0.641385845805]
    [  0.179605302027  -0.144590775798   0.925041158846  -0.301884576418]
    [  0.179605302027  -0.704880032016  0.0774617736597   0.681825307224]
    [  0.898026510134   0.397624633445 -0.0532812182975   0.180566192161]
Got:
    [ -0.359210604054   0.569326179705   0.146337376237   0.724859169325]
    [  0.179605302027  -0.144590775798   0.973035382602 0.00613084354877]
    [  0.179605302027  -0.704880032016  -0.142125402809   0.671331844787]
    [  0.898026510134   0.397624633445  -0.107647045464   0.154451130063]
**********************************************************************
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 1771:
    sage: R
Expected:
    [    -5.56776436283       2.6940795304      -2.6940795304]
    [                 0     -3.56958477752      3.56958477752]
    [                 0                  0 -9.93013661299e-16]
    [                 0                  0                  0]
Got:
    [   -5.56776436283      2.6940795304     -2.6940795304]
    [                0    -3.56958477752     3.56958477752]
    [                0                 0 6.28036983474e-16]
    [                0                 0                 0]
**********************************************************************

cleo (RHEL 5.3 ia64 Itanium 2):

**********************************************************************
File "/home/buildbot/build/sage/cleo-1/cleo_full/build/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 1766:
    sage: Q
Expected:
    [ -0.359210604054   0.569326179705   0.368048420509   0.641385845805]
    [  0.179605302027  -0.144590775798   0.925041158846  -0.301884576418]
    [  0.179605302027  -0.704880032016  0.0774617736597   0.681825307224]
    [  0.898026510134   0.397624633445 -0.0532812182975   0.180566192161]
Got:
    [ -0.359210604054   0.569326179705   0.146859067236   0.724753652911]
    [  0.179605302027  -0.144590775798   0.973039543327 0.00543048428303]
    [  0.179605302027  -0.704880032016  -0.141642164231   0.671433967908]
    [  0.898026510134   0.397624633445  -0.107535848925   0.154528570726]
**********************************************************************
File "/home/buildbot/build/sage/cleo-1/cleo_full/build/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 1771:
    sage: R
Expected:
    [    -5.56776436283       2.6940795304      -2.6940795304]
    [                 0     -3.56958477752      3.56958477752]
    [                 0                  0 -9.93013661299e-16]
    [                 0                  0                  0]
Got:
    [   -5.56776436283      2.6940795304     -2.6940795304]
    [                0    -3.56958477752     3.56958477752]
    [                0                 0 9.81879219451e-16]
    [                0                 0                 0]
**********************************************************************

@jdemeyer jdemeyer reopened this Aug 24, 2011
@jdemeyer
Copy link

comment:6

It seems that this patch causes failures in a
non-reproducible way. Is there some randomness in the algorithm?
For example, I just got this failure on sage.math.washington.edu (a
machine on which I remember the test succeeding):

sage -t -long  -force_lib devel/sage/sage/matrix/matrix_double_dense.pyx
**********************************************************************
File
"/mnt/usb1/scratch/buildbot/sage/sage-1/sage_binary/build/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx",
line 1766:
    sage: Q
Expected:
    [ -0.359210604054   0.569326179705   0.368048420509   0.641385845805]
    [  0.179605302027  -0.144590775798   0.925041158846  -0.301884576418]
    [  0.179605302027  -0.704880032016  0.0774617736597   0.681825307224]
    [  0.898026510134   0.397624633445 -0.0532812182975   0.180566192161]
Got:
    [-0.359210604054  0.569326179705  0.616028985113  0.409076682956]
    [ 0.179605302027 -0.144590775798   0.69237507842 -0.683704756325]
    [ 0.179605302027 -0.704880032016  0.374205463771  0.575201135981]
    [ 0.898026510134  0.397624633445 0.0330954856071  0.185331397251]
**********************************************************************
File
"/mnt/usb1/scratch/buildbot/sage/sage-1/sage_binary/build/sage-4.7.2.alpha2/devel/sage-main/sage/matrix/matrix_double_dense.pyx",
line 1771:
    sage: R
Expected:
    [    -5.56776436283       2.6940795304      -2.6940795304]
    [                 0     -3.56958477752      3.56958477752]
    [                 0                  0 -9.93013661299e-16]
    [                 0                  0                  0]
Got:
    [   -5.56776436283      2.6940795304     -2.6940795304]
    [                0    -3.56958477752     3.56958477752]
    [                0                 0 -4.4408920985e-16]
    [                0                 0                 0]
**********************************************************************

@rbeezer

This comment has been minimized.

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Sep 19, 2011

comment:8

"numerical" patch applies accumulated techniques for these numerical computations and should address the doctest failures.

@rbeezer rbeezer mannequin removed good first issue labels Sep 19, 2011
@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 28, 2012

Changed keywords from none to sd40.5

@rbeezer rbeezer mannequin added s: needs review and removed s: needs work labels May 28, 2012
@rbeezer

This comment has been minimized.

@sagetrac-mraum
Copy link
Mannequin

sagetrac-mraum mannequin commented May 28, 2012

comment:21

This looks good, and I think it is reasonable to use the Q as it is.

Could you please go over the patch and correct the Q and Q to Q (and I think in some places R is missing the quotes also). Then I can give this a positive review.

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 28, 2012

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 28, 2012

comment:22

Dear Martin,

Thanks for participating remotely in Sage Days 40.5. ;-)

"formatting" patch is an add-on, so you can see the changes. I did the "OUTPUT" section entirely with math quotes, other than the result objects. The rest is in code quotes.

Updated the output description, which needed work. Added some left kernel explanation for Dan Drake, who was also looking at this here at SD 40.5.

Thanks again,
Rob

@rbeezer

This comment has been minimized.

@dandrake
Copy link
Contributor

Changed reviewer from Martin Raum to Martin Raum, Dan Drake

@dandrake
Copy link
Contributor

comment:23

This is a big improvement over what we previously had. Positive review.

Just for the patchbot: apply
trac_10795-QR-decomposition-double-dense-v2.patch
trac_10795-QR-decomposition-formatting.patch

@jdemeyer
Copy link

Merged: sage-5.1.beta5

@jdemeyer
Copy link

comment:25

As reported on sage-devel, this gives doctest failures on OS X Lion, see #13140.

@jdemeyer
Copy link

Changed merged from sage-5.1.beta5 to none

@jdemeyer
Copy link

comment:26

Unmerging due to #13140.

@jdemeyer jdemeyer modified the milestones: sage-5.1, sage-5.2 Jun 26, 2012
@jdemeyer jdemeyer reopened this Jun 26, 2012
@jhpalmieri
Copy link
Member

comment:27

Should this be marked "positive review" again, since #13140 is ready?

@dandrake
Copy link
Contributor

comment:28

Replying to @jhpalmieri:

Should this be marked "positive review" again, since #13140 is ready?

I think so. If that was the only reason this got unmerged, and that ticket is now fixed, this should again be ready.

@jdemeyer
Copy link

Merged: sage-5.2.beta1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants