Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Overall looks good to me, the only thing I was wondering is if it would make sense to just remove these BLAS fallback definitions altogether. I don't know if there is any performance difference between axpy! and simple broadcasting?
As a sidenote, probably best to get this in before #54 and have a patch release before, in which case we can bump the version number in this PR already?
| ) where {T <: BlasFloat} | ||
| if β === One() | ||
| LinearAlgebra.axpy!(convert(T, α), x, y) | ||
| elseif β === Zero() || β === false |
There was a problem hiding this comment.
I'm slightly confused you also added the === false branch here, does that effectively mean that we want to interpret false as a strong zero as well?
There was a problem hiding this comment.
In Julia false is always strong zero, I thought?
There was a problem hiding this comment.
julia> 0.0 * NaN
NaN
julia> false * NaN
0.0There was a problem hiding this comment.
I did not know this 😆 I thought that was a BLAS-specific thing... In that case, ignore my comments :p
A different thing that just popped in my brain is whether it would make sense to replace these checks everywhere with isstrongzero and isstrongone, to enforce that this is handled consistently?
There was a problem hiding this comment.
I think the strong zero check is only made here, so I don't know if it makes sense to create a whole new function for it
There was a problem hiding this comment.
The reason I was thinking about this is because we also use this downstream every now and again, e.g. https://github.com/QuantumKitHub/TensorOperations.jl/blob/ef937adcfa4e6bc4476f8197a62a2da45cf3b1d9/src/implementation/strided.jl#L142 where we actually turn every zero into a strong one, and I do like the idea of having this a bit more formalized/consistent. Does not have to be in this PR though!
There was a problem hiding this comment.
I might just merge this and tag then to keep stuff moving for the other PR
3 participants
This turned out to be the origin of a crazy
NaNrelated bug in PEPSKit. The issue is that0.0 * NaN = NaN, so when VI's strong zeroZero()is converted to0.0for the GPU-basedaxpbycall, it actually gets turned into a weak zero. Not what you want! Adding a special path for this here avoids this problem. TheNaNs can occur in the first place by initializing an array withundefs.