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

Add logdet and logabsdet methods for Symmetric/Hermitian matrices #51930

Merged
merged 6 commits into from
Dec 5, 2023

Conversation

ElOceanografo
Copy link
Contributor

There was a method for det already, but not for log[abs]det.

@jishnub jishnub added the domain:linear algebra Linear algebra label Oct 30, 2023
@dkarrasch dkarrasch added the needs tests Unit tests are required for this change label Oct 31, 2023
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs tests. In particular, logabsdet returns a tuple, on which real is not defined. Another question is whether these methods are needed at all. Don't they fall back on the det implementation? It seems that all of these three methods are working, e.g., for some real Symmetric matrix.

@ElOceanografo
Copy link
Contributor Author

The logdet methods do not in general fall back on det, rather they do a lu decomposition of the matrix and call logabsdet on that. These methods are needed in particular for the case where Symmetric or Hermitian is wrapping a sparse matrix. Currently in this situation, logabsdet ends up calling a generic version of lu which is orders of magnitude slower than the specialized sparse one in UMFPACK:

using LinearAlgebra, SparseArrays
Q = sprandn(1000, 1000, 0.0002)
Q = Q'Q + I
@time logdet(Q) # 0.000180 seconds (108 allocations: 559.406 KiB)
@time logdet(Symmetric(Q)) # 5.488019 seconds (7 allocations: 82.953 KiB)

This PR seemed like the minimal change to allow the factorization method to specialize on the underlying matrix structure, though I'm open to being corrected on that!

@dkarrasch
Copy link
Member

dkarrasch commented Nov 1, 2023

Ok, the failing tests indicate that these methods are exercised. It remains to fix the logabsdet definition, perhaps using broadcasting hoist it out of the loop? Perhaps these few methods can be just spelled out directly.

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have specific tests for these cases, both real and complex, both positive and negative sign, and nail down the desired behaviour.

stdlib/LinearAlgebra/src/symmetric.jl Outdated Show resolved Hide resolved
@dkarrasch dkarrasch removed the needs tests Unit tests are required for this change label Dec 5, 2023
@dkarrasch dkarrasch merged commit fd41af5 into JuliaLang:master Dec 5, 2023
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants