Determinant of factorized matrices #1785
Merged
+410
−33
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ricardoV94
commented
Dec 10, 2025
ricardoV94
added
linalg
ricardoV94
commented
Dec 10, 2025
| match core_op: | ||
| case Cholesky(): | ||
| L = client.outputs[0] | ||
| new_det = matrix_diagonal_product(L) ** 2 |
Member
Author
There was a problem hiding this comment.
TODO: Add the positive tag here.
Possibly also rewrite for log(x ** 2) -> log(x) * 2, when we know x is positive
Member
There was a problem hiding this comment.
This all seemed out of scope so I didn't address it. Positive tagging isn't used systematically and I'd rather we have a plan for doing that.
jessegrabowski
approved these changes
Jan 3, 2026
Member
Author
|
Still missing tests and a couple other things. I wanted to first get #1786 in (which I think is still not ready either) |
Member
|
I approved to poke you towards finishing it :) |
jessegrabowski
force-pushed
the
det_rewrites
branch
from
a9d058c to
de4e763
Compare
Member
|
I used claude to add tests. I think it did a reasonable job. |
Member
|
I also rebased, so if you want to keep working on it be aware of that. |
ricardoV94
commented
Jan 9, 2026
ricardoV94
commented
Jan 9, 2026
ricardoV94
commented
Jan 9, 2026
ricardoV94
commented
Jan 9, 2026
jessegrabowski
force-pushed
the
det_rewrites
branch
from
ab26c2b to
484ab21
Compare
Co-authored-by: Jesse Grabowski <48652735+jessegrabowski@users.noreply.github.com>
Co-authored-by: Jesse Grabowski <48652735+jessegrabowski@users.noreply.github.com>
ricardoV94
force-pushed
the
det_rewrites
branch
from
484ab21 to
042db9c
Compare
Member
Author
|
Thanks for the help @jessegrabowski |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The old
local_det_cholrewrite is extended to cover more cases of a matrix that is factorized elsewhere, not just with Cholesky, but also LU, LUFactor, or SVD, QR (the latter two only if the sign isn't needed)A new rewrite is added for the determinant of a factorization itself. The logic is slightly different, for instance det(LUFactor) is non-sensical, and the determinant for some outputs of SVD/ QR can always be computed even if the determinant of the whole factorization cannot.
Also extended the rewrite of log(prod(x)) to sum(log(x)), which should increase the stability of many of these when we want the log determinant (or log(abs(determintant))).
Still missing tests
Closes #1679
Related to #573