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

det, logdet, and logabsdet rrules for SparseMatrixCSC #730

Merged
merged 8 commits into from
Aug 16, 2023

Conversation

ElOceanografo
Copy link
Contributor

This PR implements determinant rrules for unstructured sparse matrices. The equivalent rules for AbstractMatrices didn't work, since they require calculating a matrix inverse, which errors by default for sparse matrices to avoid unintentionally creating huge dense matrices. Fortunately, the adjoint for the determinant functions does not depend on all the values of the inverse matrix (see here, section 4.1 and Equation 8). Using the "inverse subset" algorithm, you can efficiently calculate just the sparse subset of the inverse matrix which you need. I just implemented that algorithm in this small package, which enabled me to write the rrules here.

I put these rules and their tests in with the "structured" matrix rule sets. Would be happy to move them to new "sparse" src and test files if that fits the organizational structure better. I am also pretty new to writing chain rules, so any feedback or corrections are more than welcome!

This PR will fix #719, and should also help fix TuringLang/DistributionsAD.jl#89.

@oxinabox
Copy link
Member

oxinabox commented Aug 15, 2023

CI is broken for this new feature on julia 1.6:
see https://github.com/JuliaDiff/ChainRules.jl/actions/runs/5812786772/job/15758921347?pr=730#step:6:300
If you can fix that, it should be good to merge.
This is pretty cool

(It's also broken on nightly for other reasons that we are going to have to look into.
But it works for this feature on nightly.)

@oxinabox
Copy link
Member

I put these rules and their tests in with the "structured" matrix rule sets.

Yes, please put them in this file:
https://github.com/JuliaDiff/ChainRules.jl/blob/main/src/rulesets/SparseArrays/sparsematrix.jl
and the tests in the matching file

@ElOceanografo
Copy link
Contributor Author

The logs do not say exactly what broke in the tests for 1.6, but I suspect it is because prior to Julia 1.7 there was no method implemented for logabsdet of sparse matrices (see this issue and PR).

If that's the problem, what's the best solution? A conditional statement that defines the missing method if VERSION < v"1.7"?

This method is required for the sparse logabsdet rrules, but was
not included in Julia prior to v1.7.
@ElOceanografo
Copy link
Contributor Author

Went ahead and added a conditional definition of the method for logabsdet that was introduced in Julia v1.7. A little hacky, but tests pass on 1.6 now. @oxinabox let me know if that seems like an ok solution...

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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

Successfully merging this pull request may close these issues.

Need for MvNormal with sparse precision matrix Errors for gradient and hessian of logabsdet of sparse matrix
2 participants