-
-
Notifications
You must be signed in to change notification settings - Fork 558
Add LDL decomposition #1515
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
base: main
Are you sure you want to change the base?
Add LDL decomposition #1515
Changes from 3 commits
5592c9f
b875935
00ca97b
d562fbd
43fd280
9731107
091d1a1
cd33a42
9bdf437
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| #[cfg(feature = "serde-serialize-no-std")] | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| use crate::allocator::Allocator; | ||
| use crate::base::{Const, DefaultAllocator, OMatrix, OVector}; | ||
| use crate::dimension::Dim; | ||
| use simba::scalar::ComplexField; | ||
|
|
||
| /// LDL factorization. | ||
| #[cfg_attr(feature = "serde-serialize-no-std", derive(Serialize, Deserialize))] | ||
| #[cfg_attr( | ||
| feature = "serde-serialize-no-std", | ||
| serde(bound(serialize = "OVector<T, D>: Serialize, OMatrix<T, D, D>: Serialize")) | ||
| )] | ||
| #[cfg_attr( | ||
| feature = "serde-serialize-no-std", | ||
| serde(bound( | ||
| deserialize = "OVector<T, D>: Deserialize<'de>, OMatrix<T, D, D>: Deserialize<'de>" | ||
| )) | ||
| )] | ||
| #[derive(Clone, Debug)] | ||
| pub struct LDL<T: ComplexField, D: Dim> | ||
| where | ||
| DefaultAllocator: Allocator<D> + Allocator<D, D>, | ||
| { | ||
| /// The lower triangular matrix resulting from the factorization | ||
| pub l: OMatrix<T, D, D>, | ||
| /// The diagonal matrix resulting from the factorization | ||
| pub d: OVector<T, D>, | ||
| } | ||
|
|
||
| impl<T: ComplexField, D: Dim> Copy for LDL<T, D> | ||
| where | ||
| DefaultAllocator: Allocator<D> + Allocator<D, D>, | ||
| OVector<T, D>: Copy, | ||
| OMatrix<T, D, D>: Copy, | ||
| { | ||
| } | ||
|
|
||
| impl<T: ComplexField, D: Dim> LDL<T, D> | ||
| where | ||
| DefaultAllocator: Allocator<D> + Allocator<D, D>, | ||
| { | ||
| /// Computes the LDL^T factorization. | ||
| /// | ||
| /// The input matrix `p` is assumed to be hermitian c and this decomposition will only read | ||
| /// the lower-triangular part of `p`. | ||
| pub fn new(p: OMatrix<T, D, D>) -> Option<Self> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is absolutely a me-problem, but is there a reason why the matrix is called
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can change this, currently the naming is just taken from the code from the UDU factorization. |
||
| let n = p.ncols(); | ||
|
|
||
| let n_dim = p.shape_generic().1; | ||
|
|
||
| let mut d = OVector::<T, D>::zeros_generic(n_dim, Const::<1>); | ||
| let mut l = OMatrix::<T, D, D>::zeros_generic(n_dim, n_dim); | ||
|
|
||
| for j in 0..n { | ||
| let mut d_j = p[(j, j)].clone(); | ||
|
|
||
| if j > 0 { | ||
| for k in 0..j { | ||
| d_j -= l[(j, k)].clone() * l[(j, k)].clone().conjugate() * d[k].clone(); | ||
| } | ||
| } | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've thought about the algorithm a bit and I'm wondering if you'd mind implementing the algorithm in #762 (comment). The reason is, that algorithm should be a more efficient in terms of memory usage (and that might make it more cache friendly, but I'm not sure) than your implementation. The reason is that the linked algorithm works in place, while your algorithm works out of place. Your interface wouldn't even have to change because you're taking the OMatrix by value already. Your original matrix A would then contain the LDL decomposition where L is in the stricly lower triangular part and d is on the diagonal. The only problem I see then, is that exposing access to L and D would require new allocations. Is that why you implemented the algorithm out of place in the first place? What I mean is this: is the primary use of LDL^T decomposition to get explicit access to L and D, or would it also be used to solve linear systems. In that case we could expose a solve method that uses the triangular solvers internally and deals with D through scaling.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could do this, but we should then change the structure a bit. LDL could then just be a new type of a single matrix without exposing the underlying field as L*D by itself does not have a use for anything. The current implementation for UDU uses the exactly same set up.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer that, unless you say the main value of the LDL(T) decomposition is access to the L and D matrix. I'd still expose access to those matrices via dedicated methods that allocate, because who know what people might use this for. I'd also add a solve method for solving linear systems in a least squares sense, that takes advantage of the internal structure.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code has ben re-factored to make the calculations in-place, I'm still using the same algorithm just not allocating new matrices. |
||
| d[j] = d_j; | ||
|
|
||
| for i in j..n { | ||
| let mut l_ij = p[(i, j)].clone(); | ||
|
|
||
| for k in 0..j { | ||
| l_ij -= l[(j, k)].clone().conjugate() * l[(i, k)].clone() * d[k].clone(); | ||
| } | ||
|
|
||
| if d[j] == T::zero() { | ||
| l[(i, j)] = T::zero(); | ||
| } else { | ||
| l[(i, j)] = l_ij / d[j].clone(); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it actually correct to do that or should we return
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comment above about the choice for semi-positive/negative definite matrices.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry for being a bit dense here (no pun intended), but are you saying that your algorithm works for all symmetric positive or negative semidefinite matrices? What if you changed the algorithm to use the in-place algorithm as suggested below? That algorithm requires A to have an LU decomp, which I believe precludes semi-definiteness... |
||
| } | ||
| } | ||
|
|
||
| Some(Self { l, d }) | ||
| } | ||
|
|
||
| /// Returns the lower triangular matrix as if generated by the Cholesky decomposition. | ||
| pub fn cholesky_l(&self) -> OMatrix<T, D, D> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather not have this in there since it's easy to google how to obtain an LL^T decomposition from an LDL^T decomposition. I don't have super strong feelings about this, but the naming is a bit awkward and the use case seems pretty fringe. But if you tell me this absolutely needs to be in there, I'll concede.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a better name for this function would be One of the main uses cases of this factorization is to get the same result as from Cholesky but without the same constraints. |
||
| let n_dim = self.l.shape_generic().1; | ||
|
|
||
| &self.l | ||
| * OMatrix::from_diagonal(&OVector::from_iterator_generic( | ||
| n_dim, | ||
| Const::<1>, | ||
| self.d.iter().map(|value| value.clone().sqrt()), | ||
| )) | ||
| } | ||
|
|
||
| /// Returns the diagonal elements as a matrix | ||
| #[must_use] | ||
| pub fn d_matrix(&self) -> OMatrix<T, D, D> { | ||
| OMatrix::from_diagonal(&self.d) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| use na::{Complex, Matrix3}; | ||
| use num::Zero; | ||
|
|
||
| #[test] | ||
| #[rustfmt::skip] | ||
| fn ldl_simple() { | ||
| let m = Matrix3::new( | ||
| Complex::new(2.0, 0.0), Complex::new(-1.0, 0.5), Complex::zero(), | ||
| Complex::new(-1.0, -0.5), Complex::new(2.0, 0.0), Complex::new(-1.0, 0.0), | ||
| Complex::zero(), Complex::new(-1.0, 0.0), Complex::new(2.0, 0.0)); | ||
|
|
||
| let ldl = m.lower_triangle().ldl().unwrap(); | ||
|
|
||
| // Rebuild | ||
| let p = ldl.l * ldl.d_matrix() * ldl.l.adjoint(); | ||
|
|
||
| assert!(relative_eq!(m, p, epsilon = 3.0e-12)); | ||
| } | ||
|
|
||
| #[test] | ||
| #[rustfmt::skip] | ||
| fn ldl_partial() { | ||
| let m = Matrix3::new( | ||
| Complex::new(2.0, 0.0), Complex::zero(), Complex::zero(), | ||
| Complex::zero(), Complex::zero(), Complex::zero(), | ||
| Complex::zero(), Complex::zero(), Complex::new(2.0, 0.0)); | ||
|
|
||
| let ldl = m.lower_triangle().ldl().unwrap(); | ||
|
|
||
| // Rebuild | ||
| let p = ldl.l * ldl.d_matrix() * ldl.l.adjoint(); | ||
|
|
||
| assert!(relative_eq!(m, p, epsilon = 3.0e-12)); | ||
| } | ||
|
|
||
| #[test] | ||
| #[rustfmt::skip] | ||
| fn ldl_cholesky() { | ||
| let m = Matrix3::new( | ||
| Complex::new(2.0, 0.0), Complex::new(-1.0, 0.5), Complex::zero(), | ||
| Complex::new(-1.0, -0.5), Complex::new(2.0, 0.0), Complex::new(-1.0, 0.0), | ||
| Complex::zero(), Complex::new(-1.0, 0.0), Complex::new(2.0, 0.0)); | ||
|
|
||
| let chol= m.cholesky().unwrap(); | ||
| let ldl = m.ldl().unwrap(); | ||
|
|
||
| assert!(relative_eq!(ldl.cholesky_l(), chol.l(), epsilon = 3.0e-16)); | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic] | ||
| #[rustfmt::skip] | ||
| fn ldl_non_sym_panic() { | ||
| let m = Matrix3::new( | ||
| 2.0, -1.0, 0.0, | ||
| 1.0, -2.0, 3.0, | ||
| -2.0, 1.0, 0.3); | ||
|
|
||
| let ldl = m.ldl().unwrap(); | ||
|
|
||
| // Rebuild | ||
| let p = ldl.l * ldl.d_matrix() * ldl.l.transpose(); | ||
|
|
||
| assert!(relative_eq!(m, p, epsilon = 3.0e-16)); | ||
| } | ||
|
|
||
| #[cfg(feature = "proptest-support")] | ||
| mod proptest_tests { | ||
| #[allow(unused_imports)] | ||
| use crate::core::helper::{RandComplex, RandScalar}; | ||
|
|
||
| macro_rules! gen_tests( | ||
| ($module: ident, $scalar: expr) => { | ||
| mod $module { | ||
| #[allow(unused_imports)] | ||
| use crate::core::helper::{RandScalar, RandComplex}; | ||
| use crate::proptest::*; | ||
| use proptest::{prop_assert, proptest}; | ||
|
|
||
| proptest! { | ||
| #[test] | ||
| fn ldl(m in dmatrix_($scalar)) { | ||
| let m = &m * m.adjoint(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understood correctly what you said in the comment, then this implementation of the LDL^T decomposition requires the matrix to be symmetric positive definite (not semi definite), right? However, using
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just wanted to weigh in - i'm a little confused actually... the 'point' of LDL is that it's a generalisation of the Cholesky algorithm to all symmetric/Hermitian matrices, with no requirements on positive definiteness/semi-definiteness. If it only works for definite/semidefinite matrices then there's no benefit to using LDL over Cholesky, which is a simpler decomposition and a more efficient algorithm.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry if the above text is slightly confusing. But no, the LDL/LDL^T decomposition only requires the matrix to be symmetric. It does NOT require it to be positive definite. This is stated in both textbooks referenced here. The problem is that, in both cases, the algorithms that are shown DO require the matrix to be positive OR negative definite. I'd have to look deeper into this, but I think this is because in the semi-positive/negative definite cases the solution is not unique. I think there is a choice for the particular column where the corresponding value in the D matrix is zero.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for working on this! Having an LDL-like factorization in nalgebra would be genuinely useful. As written, this looks to me like the standard no-pivot, strictly diagonal LDLᵀ/LDLᴴ recursion, rather than the more general symmetric-indefinite factorization many users may expect from an To handle symmetric/Hermitian-indefinite factorizations, you have to use the Bunch–Kaufman family (or equivalent): symmetric pivoting/permutations, with In particular, without pivoting and with Because of that, I’d be hesitant to expose this as a general-purpose
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @awinick, thanks for weighing in! Do you have a source for the symmetric indefinite LDL decomposition that you mentioned?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original '77 paper is openly available here https://www.ams.org/journals/mcom/1977-31-137/S0025-5718-1977-0428694-0/S0025-5718-1977-0428694-0.pdf. Also see the comment here #1515 (comment) Personally I would prefer continuing with option (1) for the time being as it seems to cover most practical use cases that I have run into. I can clean up the code / docs a bit more at some later time when I have more free time.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @geo-ant you might be interested in the PR I just opened. |
||
|
|
||
| if let Some(ldl) = m.clone().ldl() { | ||
| let p = &ldl.l * &ldl.d_matrix() * &ldl.l.transpose(); | ||
| println!("m: {}, p: {}", m, p); | ||
|
|
||
| prop_assert!(relative_eq!(m, p, epsilon = 1.0e-7)); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn ldl_static(m in matrix4_($scalar)) { | ||
| let m = m.hermitian_part(); | ||
|
|
||
| if let Some(ldl) = m.ldl() { | ||
| let p = ldl.l * ldl.d_matrix() * ldl.l.transpose(); | ||
| prop_assert!(relative_eq!(m, p, epsilon = 1.0e-7)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| gen_tests!(f64, PROPTEST_F64); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ mod exp; | |
| mod full_piv_lu; | ||
| mod hessenberg; | ||
| mod inverse; | ||
| mod ldl; | ||
| mod lu; | ||
| mod pow; | ||
| mod qr; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you expand this to include more about the pre-and-postconditions? For example:
Ainto a lower unit-triangular (??) matrixLand a diagonal matrixDsuch thatA = LDL^T".A. I think it must be Hermitian?It's fine to keep it concise, but I find it helpful to have all the important info right there from a user's perspective.
Thinking about this some more, do you think this decomposition should be called LDL or LDLT? I'm unsure...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just want to add to make sure we clarify that
Dis strictly diagonal - people also use LDL to refer to a different algorithm which decomposes a Hermitian matrix into a lower unit-triangular matrix L and a block-diagonal matrixDcomposed of 1x1 and 2x2 blocks.This is the algorithm implemented in the *HETRF family of algorithms in LAPACK, which are wrapped by SciPy's
ldlfunction in Python as well as MATLAB'sldlfunction, so users may be expecting the block-diagonal algorithm and be confused. (the idea of the block-diagonal algorithm is that there are bounds on the size of the elements ofDwhich do not exist for the strictly diagonal algorithm)The Rust package
faerhas both versions - they call the strictly diagonal one LDLT and the block-diagonal one LBLT.for some comparisons summarising info above, both Eigen (C++) and
faercall it LDLT, SciPy calls it LDL, as does MATLAB.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments.
Correction: My implementation of the algorithm covers semi-positive (and semi-negative) definite matrices. The referenced algorithms in the textbooks DO NOT.
Regarding the naming convection LDL/LDLT. As @alexhroom pointed out, both conventions are used. Given the fact that we already have the UDU factorization implemented as UDU (and not UDU^T), I think it would make sense to keep it as LDL.