New logm implementation #21571
New logm implementation #21571
Conversation
base/linalg/triangular.jl
Outdated
| for j = 3:tmax | ||
| if alpha3 <= theta[j] | ||
| break | ||
| m = 0; m += (alpha2 <= theta[1] ? 1 : 2) |
There was a problem hiding this comment.
why the = 0 then immediately += ?
| # computes the principal square root X | ||
| # of the matrix A using the product form of the Denman-Beavers | ||
| # iteration. The matrix M tends to I. | ||
| # Copyright (c) 2008 by N. J. Higham |
There was a problem hiding this comment.
he's been very nice before about granting permission to relicense derived translations of his implementations, did a previous instance of that also cover this bit of code?
There was a problem hiding this comment.
Wellsqrtm_dbp is internally called by logm, for which he has given permission to use. So yes, I guess.
cc @higham
| @@ -473,7 +473,7 @@ for (funm, func) in ([:logm,:log], [:sqrtm,:sqrt]) | |||
| @eval begin | |||
| function ($funm){T<:Real}(A::Symmetric{T}) | |||
| F = eigfact(A) | |||
| if isposdef(F) | |||
| if all(λ -> λ ≥ 0, F.values) | |||
There was a problem hiding this comment.
Why this change? isposdef(F) is more clear IMO and does the same thing? Same just below too.
Edit: Nevermind, isposdef checks λ > 0
|
Would be good to add the cases from JuliaLang/LinearAlgebra.jl#415 as tests, and maybe a handful more that are wrong or broken on master but fixed here. |
| @@ -551,40 +551,26 @@ julia> logm(A) | |||
| 0.0 1.0 | |||
| ``` | |||
| """ | |||
| function logm(A::StridedMatrix{T}) where T | |||
| function logm{T}(A::StridedMatrix{T}) | |||
There was a problem hiding this comment.
this should go back the way it was
There was a problem hiding this comment.
For future reference, is there something wrong with this, or is it just the preferred syntax?
There was a problem hiding this comment.
the logm{T} version is likely to be deprecated for 0.7/1.0
| @@ -454,31 +454,55 @@ end | |||
| A5 = convert(Matrix{elty}, [1 1 0 1; 0 1 1 0; 0 0 1 1; 1 0 0 1]) | |||
| @test expm(logm(A5)) ≈ A5 | |||
|
|
|||
| A6 = convert(Matrix{elty}, [-5 2 0 0 ; 1/2 -7 3 0; 0 1/3 -9 4; 0 0 1/4 -11]) | |||
| @test expm(logm(A6)) ≈ A6 | |||
There was a problem hiding this comment.
why remove this one?
(likewise with @test (Ab^im)^(-im) ≈ Ab)
There was a problem hiding this comment.
Because A6, Ab have negative eigenvalues so logm is not unique and the logarithm returned on these two occasions isn't one that expm(logm(A)) == A.
(Ab^im = expm(im*logm(A)))
There was a problem hiding this comment.
so the new algorithm returns a different root? should we test the new result instead?
There was a problem hiding this comment.
I'm not sure how to test that
There was a problem hiding this comment.
compare the result against its expected value written out? is the root this algorithm now returns consistent with what any other libraries return, like wolfram or matlab or octave, or would we be making a new outlier choice?
There was a problem hiding this comment.
Now I'm not following. Even if logm(A) isn't unique, any such solution should satisfy expm(logm(A)) == A, shouldn't it? How is it a valid logm(A) otherwise?
There was a problem hiding this comment.
Yes yes, my previous comment was wrong. If A has negative eigenvalues, logm is not unique and this algorithm's results may be inaccurate.
I'll work on it more.
|
It appears that the previous implementation was better and the problem was elsewhere. |
|
Yeap, that's why I closed this PR. |
|
Yeah, I tried to write a PR, and I cannot figure it out. |
Fixed 21179
Original algorithm can be found here.