Replace Unitful aware givensAlgorithm with a generic version. #54078
Conversation
andreasnoack
added
domain:linear algebra
andreasnoack
force-pushed
the
an/genericgivens
branch
from
98377c8
to
2828fe6
Compare
|
how is the performance of this? it doesn't look like it would be fast. |
|
Since it's a generic fallback, performance wasn't really the focus, but I'm curious what specifically makes you think that it's slow. |
andreasnoack
force-pushed
the
an/genericgivens
branch
from
2828fe6
to
d367449
Compare
|
the main thing that seems slow is the division to get sign. division is pretty slow. |
andreasnoack
force-pushed
the
an/genericgivens
branch
from
d367449
to
0911173
Compare
The implementation is based on Janovská, D., & Opfer, G. (2003) "Givens’ Transformation Applied to Quaternion Valued Vectors."
andreasnoack
force-pushed
the
an/genericgivens
branch
from
0911173
to
04ef0f6
Compare
| nrm = hypot(f, g) | ||
| c = abs(f) / nrm | ||
| s, u = if iszero(f) | ||
| -one(first(promote(f, g))), -g |
There was a problem hiding this comment.
I just noticed that this is inconsistent with the existing definition for Float64:
julia> using LinearAlgebra
julia> LinearAlgebra.givensAlgorithm(0.0, 2.0)
(0.0, 1.0, 2.0)It seems to be consistent with the existing definitions we would need something like
| -one(first(promote(f, g))), -g | |
| one(first(promote(f, g))), g |
There was a problem hiding this comment.
I'm not sure it's worth trying to match the values of c and s from the two versions translated from LAPACK. Then we'd essentially have to reimplement the version for complex numbers and I'm not sure if it necessarily works for quaternions. In any case, which of the allowed values of c and s doesn't really matter. It's always the result of applying the rotation that matters and it isn't affected by this choice (except for rounding).
There was a problem hiding this comment.
Thought a bit more about this and concluded that you are right here. The fallback would have to make the same sign decisions as the specialized version so the approach of this PR can't work. We should probably try to make a version of the complex version that isn't restricted to Complex32/64.
The implementation is based on
Janovská, D., & Opfer, G. (2003) "Givens’ Transformation Applied to Quaternion Valued Vectors."
Fixes #41753