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

Positive definite check for matrices over RDF/CDF #13052

Closed
rbeezer mannequin opened this issue May 28, 2012 · 14 comments
Closed

Positive definite check for matrices over RDF/CDF #13052

rbeezer mannequin opened this issue May 28, 2012 · 14 comments

Comments

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented May 28, 2012

Having a Cholesky decomposition is equivalent to being positive definite. With #13035 this is a cheap feature and a cheap computation, and will maintain feature sets with the exact case.

Depends: #13035

Apply:

  1. attachment: trac_13052-is-positive-definite-RDF-v2.patch

Depends on #13035

CC: @dandrake

Component: linear algebra

Keywords: sd40.5

Author: Rob Beezer

Reviewer: Dan Drake, Andrey Novoseltsev

Merged: sage-5.2.beta0

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

@rbeezer rbeezer mannequin added this to the sage-5.1 milestone May 28, 2012
@rbeezer rbeezer mannequin assigned jasongrout and williamstein May 28, 2012
@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 28, 2012

Changed keywords from none to sd40.5

@rbeezer

This comment has been minimized.

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 28, 2012

Author: Rob Beezer

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 28, 2012

Dependencies: #13035

@rbeezer rbeezer mannequin added the s: needs review label May 28, 2012
@novoselt
Copy link
Member

comment:2

One too many squares in "A square matrix A is positive definite if it is square,"

@novoselt
Copy link
Member

comment:3

And I am having the same problem as the patchbot applying it - something is wrong with dependencies?

@dandrake
Copy link
Contributor

comment:4

Replying to @novoselt:

And I am having the same problem as the patchbot applying it - something is wrong with dependencies?

No, there really is a conflict with William's referee patch at #13035. Shouldn't be too hard to fix up.

@dandrake
Copy link
Contributor

comment:5

Without the referee patch from #13035 (which does not affect functionality), I get some numerical noise on 64-bit Ubuntu 10.04:

sage -t  devel/sage/sage/matrix/matrix_double_dense.pyx
**********************************************************************
File "/home/drake/s/sage-5.1.beta0/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 3537:
    sage: [M[:i,:i].determinant() for i in range(1, M.nrows()+1)]
Expected:
    [1.0, 4.0, 460.0, 27936.0, 82944.0]
Got:
    [1.0, 4.0, 460.0, 27936.0, 82943.9999998]
**********************************************************************

That's from an integer matrix, so putting a round() in there will not harm anything.

@rbeezer

This comment has been minimized.

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 29, 2012

comment:6

Attachment: trac_13052-is-positive-definite-RDF-v2.patch.gz

Thanks, Dan and Andrey. v2 patch is standalone patch.

  • Rebased with missed reviewer patch from other ticket.
  • Double "square" is gone.
  • round() on all integer determinants and output reformatted

Thanks for all your work on the Cholesky stack!

Rob

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 29, 2012

Reviewer: Dan Drake, Andrey Novoselt

@novoselt
Copy link
Member

Changed reviewer from Dan Drake, Andrey Novoselt to Dan Drake, Andrey Novoseltsev

@dandrake
Copy link
Contributor

comment:8

Patchbot, only apply trac_13052-is-positive-definite-RDF-v2.patch.

@jdemeyer jdemeyer changed the title Positive definte check for matrices over RDF/CDF Positive definite check for matrices over RDF/CDF May 29, 2012
@jdemeyer jdemeyer modified the milestones: sage-5.1, sage-5.2 Jun 18, 2012
@jdemeyer
Copy link

jdemeyer commented Jul 2, 2012

Merged: sage-5.2.beta0

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