-
-
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
Adding specialized ≈ and norm for structured matrices. #29724
base: master
Are you sure you want to change the base?
Adding specialized ≈ and norm for structured matrices. #29724
Conversation
stdlib/LinearAlgebra/src/special.jl
Outdated
rtol::Real=Base.rtoldefault(promote_leaf_eltypes(A),promote_leaf_eltypes(B),atol), | ||
nans::Bool=false, norm::Function=norm) | ||
|
||
return iszero(B.ev) && isapprox(A.diag, B.dv; rtol=rtol, atol=atol, nans=nans) |
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.
Should this be a test for approximately zero when atol!=0?
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.
The norm argument is unused
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.
For vecnorm
it would be hypot(vecnorm(B.ev), vecnorm(A.diag .- B.dv)) <= ...
, but to deal with the general case I guess you need a fallback.
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.
vecnorm
was deprecated with v0.7. The problem with using a recursive isapprox
fallback is that isapprox
doesn't have a kwarg for norm
when the eltype is Number
.
This needs a rebase. |
@mcognetta would you be able to rebase this one? |
Yes, I will try. I haven't looked at this in quite a while though. Does it otherwise seem ok? |
Same here. I looked at the list of PRs that I was requested for review I found this old one.
Yeah. I think so. |
the
==
half of this PR was moved to #30108.The logic for
isapprox
here is not correct yet. The way I plan to proceed is to implement specializednorm
for structured matrices and then the fallback to the genericisapprox
can be made much faster even if it does not fall back to element wise comparison.Currently,
==
and≈
fall back to generic methods. This PR adds special methods for structured matrix types. It also fixes an error inbidiag.jl
where==
fails:This PR
Some timings (these are pessimal cases):
v1.0
This PR
(Forgot to time
≈
):v1.0
This PR