-
-
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
Use zero in istriu, istril methods #19399
Conversation
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.
This is the right direction. We have wrongly been relying on 0
for some time. I have a few comments in the code.
@@ -263,8 +263,8 @@ end | |||
transpose(M::Bidiagonal) = Bidiagonal(M.dv, M.ev, !M.isupper) | |||
ctranspose(M::Bidiagonal) = Bidiagonal(conj(M.dv), conj(M.ev), !M.isupper) | |||
|
|||
istriu(M::Bidiagonal) = M.isupper || all(M.ev .== 0) | |||
istril(M::Bidiagonal) = !M.isupper || all(M.ev .== 0) | |||
istriu(M::Bidiagonal) = M.isupper || all(M.ev .== zero(eltype(M))) |
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.
I'd make this one
all(t -> t == zero(t), M.ev)
because then you use the instance instead of the type to create the zero. Not all types support zero
, e.g. Matrix
.
for j = 1:min(n,m-1), i = j+1:m | ||
if A[i,j] != 0 | ||
if A[i,j] != z |
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.
Use A[i,j] != zeros(A[i,j])
for the same reason as before and since the element type could potentially change over the array. This would also fail if the array was a Vector{Any}
.
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.
probably a typo but I guess zero(A[i,j])
instead of zeros
.
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.
Indeed
See the issues discussed in #9325. They all still apply, so I don't think this is so simple. |
After reading #9325, +1 for an
|
@tkelman Thanks for the info, I wasn't aware of some of the subtleties. I'll close this for now. |
This PR replaces
0
withzero(T)
for appropriateT
in methods ofistriu
andistril
. I'm not sure what tests to add if any since the behavior is unchanged for subtypes ofNumber
found in Base, and there are already some tests for these methods. I'm happy to add some if there are suggestions. I was unable to detect any regressions from these changes using BaseBenchmarks and BenchmarkTools.For context, I noticed this when playing around with matrices of Unitful numbers, where comparisons with
0
are problematic.