-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
deprecate sqrtm in favor of sqrt #23504
Conversation
test/linalg/dense.jl
Outdated
@@ -676,7 +676,7 @@ end | |||
a = rand(elty) | |||
@test exp(a) == exp(a) | |||
@test isposdef(one(elty)) | |||
@test sqrtm(a) == sqrt(a) | |||
@test sqrt(a) == sqrt(a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably drop this test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might as well drop the exp(a) == exp(a)
one too while at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If those tests fail, we know something more fundamentally, existentially wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we do also test @test true
and @test !false
, etc.
One thing that bothers me about the new behavior is a way in which functions with a branch cut now behave differently between scalars and matrices. For instance, julia> sqrt(diagm([-4]))
1×1 Array{Complex{Float64},2}:
0.0 + 2.0im
julia> sqrt(-4)
ERROR: DomainError with -4.0:
sqrt will only return a complex result if called with a complex argument. Try sqrt(Complex(x)). I did not see any mention of this in the discussion (though I have may missed it -- there is a lot of discussion, but much is for This seems pretty minor to me, but I'm trying to think whether this behavior difference might have any meaningful consequences when writing generic code. |
It would definitely be good to see if we can get the branch cuts to match between scalars and matrices. That's the kind of attention to detail that we're into around here :) |
Thanks all! :) |
This pull request deprecates the spelling
sqrtm
for matrix square root in favor ofsqrt
. Followup to the much-debated #23233. Also ref. #19598 and #8450. Best!