Add eig_trunc_no_error and eigh_trunc_no_error
#117
Conversation
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
kshyatt
force-pushed
the
ksh/eigs_trunc
branch
from
ebd2b55 to
1dc2cd0
Compare
| s = 1 - sqrt(eps(real(T))) | ||
| trunc = truncerror(; atol = s * norm(@view(D₀[r:end]), 1), p = 1) | ||
| D4, V4 = @constinferred eig_trunc_no_error(A; alg, trunc) | ||
| @test length(diagview(D4)) == r |
There was a problem hiding this comment.
I am a bit confused by this logic. You want an error that is slightly smaller than the norm of D₀[r:end], which means D₀[r] still needs to be kept. I would find it more intuitive to say that you want a truncation error that is allowed to be slightly bigger than D₀[r+1:end]. But maybe that is just me 🙂 .
There was a problem hiding this comment.
Basically, my reasoning would be: I want D₀[r+1:end] to be truncated, so I admit an error atol = norm(D₀[r+1:end]). But then I multiply this with a factor slightly bigger than 1 to account for finite precision errors.
That seems one less mental step than saying, I want an error that is slightly smaller than norm(D₀[r:end]), so actually, I do still want to keep D₀[r] and only start to truncate from r+1 onwards.
There was a problem hiding this comment.
Hmm, I see what you mean. But I suppose this is the "already existing" test so maybe we should open an issue about this to deal with it separately?
There was a problem hiding this comment.
Sure, now that I get the logic, it is also fine with me as is. Not sure why I didn't stumble on this before.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
Very similar reasoning as #116 . Have added AD rules for the new functions and hopefully covered them in tests.