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

make log_quasitriu always take at least one sqrt #54867

Closed
wants to merge 1 commit into from

Conversation

oscardssmith
Copy link
Member

seems to fix #54833 (although it's not a fully principled fix, I just think this might have been an edge-case neglected in the original paper, that said, it also possibly is a problem with m>4 that is independent of the number of sqrts we take).

Before:

julia> qtriu = [0.9950041652780259 -0.09983341664682815 0.07153667745275474; 0.09983341664682804 0.9950041652780259 0.06981527929449967; 0.0 0.0 1.0]
#before
julia> exp(log(qtriu)) - qtriu
3×3 Matrix{Float64}:
 1.11022e-16  1.38778e-17  -0.00269465
 0.0          1.11022e-16   0.00260462
 0.0          0.0           0.0
 #after
 3×3 Matrix{Float64}:
 1.11022e-16  1.38778e-17  3.99319e-13
 0.0          1.11022e-16  3.8973e-13
 0.0          0.0          0.0

This still needs tests.

@dkarrasch dkarrasch added the linear algebra Linear algebra label Jun 20, 2024
@oscardssmith oscardssmith added the bugfix This change fixes an existing bug label Jun 21, 2024
@dkarrasch
Copy link
Member

Interestingly, the scipy version doesn't seem to have this IIUC: https://github.com/scipy/scipy/blob/628a6b3bf3e1a3149b8a6390322ae976bba930fa/scipy/linalg/_matfuncs_inv_ssq.py#L367-L377

@oscardssmith
Copy link
Member Author

Ah, that is a very useful resource. We should definitely check that out to see what the correct params are.

@giordano giordano added the needs tests Unit tests are required for this change label Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug linear algebra Linear algebra needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Matrix exp and log are not inverse of each other
3 participants