-
Notifications
You must be signed in to change notification settings - Fork 37
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
Fix slerp
and deprecate linpol
#78
Conversation
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
==========================================
- Coverage 97.27% 97.21% -0.07%
==========================================
Files 3 3
Lines 404 395 -9
==========================================
- Hits 393 384 -9
Misses 11 11
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
I actually quite like this option. Although, I'll note that this expression is not quite correct. If |
Thanks for the correction! The
is also not quite correct with current julia> using Quaternions
julia> q1 = Quaternion(1.,0,0,0)
QuaternionF64(1.0, 0.0, 0.0, 0.0, false)
julia> q2 = Quaternion(-1/√2,-1/√2,0,0)
QuaternionF64(-0.7071067811865475, -0.7071067811865475, 0.0, 0.0, false)
julia> t = 1.0
1.0
julia> slerp(q1, q2, t)
QuaternionF64(0.7071067811865475, 0.7071067811865475, 0.0, 0.0, false)
julia> linpol(q1, q2, t)
QuaternionF64(0.7071067811865476, 0.7071067811865476, 0.0, 0.0, true)
julia> q1*(q2/q1)^t
QuaternionF64(-0.7071067811865474, -0.7071067811865475, 0.0, 0.0, false) The current
|
If we have the antipodal identification, then the magnitude interpolation might be useless, I guess. If someone needs the interpolation, one can just evaluate |
When would this not be sensible? The unit quaternions doubly cover the rotations, so that
I don't think so. The non-zero quaternions can be interpreted as the direct product of two manifolds: the (right-)quaternionic projective space (the hemisphere of unit quaternions corresponding to rotations) and the scale group (strictly positive reals). This is the manifold on which the
When I benchmarked this, I found it to be quite inefficient compared to I suppose I haven't argued for the benefits of doing it this way. Here are a few:
I don't think this is super important, and if you feel very strongly that |
The sphere S³ ≃ SU(2) ≃ (set of unit quaternions) is not just a double cover of SO(3) but also a universal cover. This means each point on S³ can be regarded as a path (or equivalence class of paths) in SO(3). These videos might be helpful to understand the coverings: Therefore, one quaternion can be interpreted as a path of rotation, so I think it would be useful to distinguish q and -q (with an optional argument like
I was thinking that unit quaternions is just a subset of ℍ, but I now agree with the interpolation of magnitude! (ℍ-{0} ≃ ℝ₊ × S³) However, that will be a breaking change with |
I understand S³ universally covers SO(3). I do not understand the assertion that each point on S³ is a path on SO(3), and I did not see this assertion made in any of the provided links. I also don't understand how this relates to the points I made above about what slerp is useful for. Are you aware of any descriptions of slerp that explicitly draw paths between quaternions from different hemispheres (as opposed to omitting the identification of antipodal quaternions for simplicity of exposition)? I understand when considering the unit quaternions as a manifold (or considering any manifold for that matter), this is something one might want to do, but slerp is concerned with 3D rotations, hence why it interpolates on a single hemisphere.
Ah, yes, then this is the better way to go for now. |
Sorry for the late reply.
A universal covering manifold M̃ of a manifold M can be constructed by the following quotient set. See section 1.5 (d) in Shigeyuki Morita, Geometry of Differential Forms for more infomation.
Sorry for the insufficient explanation. I was trying to explain that the 0° rotation and 360° rotation are different, but 0° and 720° are equivalent because a path from 0° to 720° is contractible.
If a robot has a arm like human body, then the rotation 90° and -270° should be distinguishable because the path is not contractible, and one might need different slerps |
To be clarified, I made two misunderstandings comments:
I now think that it would be better to have an optional argument like |
I merged the What I have done in this PR:Fix bug in
|
I've also added Current master julia> slerp(quat(1),quat(0),1)
QuaternionF64(0.0, 0.0, 0.0, 0.0, false) With this PR julia> slerp(quat(1),quat(0),1)
ERROR: DomainError with Quaternion{Int64}(0, 0, 0, 0, false):
The input quaternion must be non-zero.
Stacktrace:
[1] slerp(qa::Quaternion{Int64}, qb::Quaternion{Int64}, t::Int64)
@ Quaternions ~/.julia/dev/Quaternions/src/Quaternion.jl:312
[2] top-level scope
@ REPL[2]:1 |
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.
Overall code looks good, and I agree on the scope of this PR. I made a few suggestions. Also, we need tests for the three added features:
- the bug fix
norm=true
linpol
is deprecated
Co-authored-by: Seth Axen <seth.axen@gmail.com>
I've added the following tests.
|
Ohh, I found some performance degressions. current master julia> using Quaternions, BenchmarkTools
julia> q1,q2 = normalize.(rand(QuaternionF64,2))
2-element Vector{QuaternionF64}:
QuaternionF64(0.13376212455218736, 0.8893671325737943, 0.2937038326951311, 0.32383924436816264, true)
QuaternionF64(0.4630216872108008, 0.4083229516075815, 0.7029702168934131, 0.35315174999391385, true)
julia> @benchmark slerp($q1,$q2,0.2)
BenchmarkTools.Trial: 10000 samples with 994 evaluations.
Range (min … max): 33.141 ns … 57.069 ns ┊ GC (min … max): 0.00% … 0.00%
Time (median): 34.058 ns ┊ GC (median): 0.00%
Time (mean ± σ): 34.450 ns ± 1.993 ns ┊ GC (mean ± σ): 0.00% ± 0.00%
▁▂█ ▃
▃███▄▇█▅▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▂▂▂▂▁▁▁▁▁▂▂▂▂▁▂▂▂▁▂▁▂▂▂ ▃
33.1 ns Histogram: frequency by time 46.9 ns <
Memory estimate: 0 bytes, allocs estimate: 0.
julia> @benchmark linpol($q1,$q2,0.2)
BenchmarkTools.Trial: 10000 samples with 989 evaluations.
Range (min … max): 46.063 ns … 62.636 ns ┊ GC (min … max): 0.00% … 0.00%
Time (median): 47.106 ns ┊ GC (median): 0.00%
Time (mean ± σ): 47.183 ns ± 0.779 ns ┊ GC (mean ± σ): 0.00% ± 0.00%
▁ █ ▅▂
▃▁▂▃▁▁▄█▁▆▇▁▃█▂▂██▂▂▇▁▂▂▂▂▂▂▁▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂ ▃
46.1 ns Histogram: frequency by time 50.6 ns <
Memory estimate: 0 bytes, allocs estimate: 0. with this PR julia> using Quaternions, BenchmarkTools
julia> q1,q2 = normalize.(rand(QuaternionF64,2))
2-element Vector{QuaternionF64}:
QuaternionF64(0.26545001572497234, 0.31425231331646797, 0.6605279482815369, 0.6280800922381187, true)
QuaternionF64(0.21867092776216737, 0.6423537858021358, 0.6971969708881615, 0.23125964412683261, true)
julia> @benchmark slerp($q1,$q2,0.2)
BenchmarkTools.Trial: 10000 samples with 981 evaluations.
Range (min … max): 61.737 ns … 175.857 ns ┊ GC (min … max): 0.00% … 0.00%
Time (median): 62.493 ns ┊ GC (median): 0.00%
Time (mean ± σ): 63.211 ns ± 2.643 ns ┊ GC (mean ± σ): 0.00% ± 0.00%
▅ █ ▄ ▃ ▁
█▂▆▃█▃█▅▅▅▅▅▅▅▅▆▅▆▆▆▆▆▆▇▆▇▇▇▆▆▅▅▅▄▅▄█▆█▅▄█▅█▆▅▆▅▆▅▅▄▄▄▅▄▄▄▅▅ █
61.7 ns Histogram: log(frequency) by time 72.3 ns <
Memory estimate: 0 bytes, allocs estimate: 0.
julia> @benchmark linpol($q1,$q2,0.2)
BenchmarkTools.Trial: 10000 samples with 964 evaluations.
Range (min … max): 83.602 ns … 132.761 ns ┊ GC (min … max): 0.00% … 0.00%
Time (median): 85.369 ns ┊ GC (median): 0.00%
Time (mean ± σ): 85.623 ns ± 1.951 ns ┊ GC (mean ± σ): 0.00% ± 0.00%
▁▄▄▃▃▄▅██▂ ▁ ▂
███████████▇▆▆▆██████▇█▇████▇▅▄▄▄▄▄▅▄▄▄▅▁▄▁▄▃▁▁▄▁▄▁▁▁▄▃▁▁▆▄▇ █
83.6 ns Histogram: log(frequency) by time 97.1 ns <
Memory estimate: 0 bytes, allocs estimate: 0. |
Can the difference be explained by the normalization? On my machine the performance regression is less extreme, but adding the |
…oving unnecessary semi-colon
Yeah, the problem was around normalization with julia> using Quaternions, BenchmarkTools
julia> q1,q2 = normalize.(rand(QuaternionF64,2))
2-element Vector{QuaternionF64}:
QuaternionF64(0.495912700698677, 0.47929964850623047, 0.5228646779504917, 0.5009540585515571, true)
QuaternionF64(0.7828859145095284, 0.27796675430678036, 0.49666698530615594, 0.25128874640466514, true)
julia> @benchmark slerp($q1,$q2,0.2)
BenchmarkTools.Trial: 10000 samples with 991 evaluations.
Range (min … max): 40.218 ns … 63.298 ns ┊ GC (min … max): 0.00% … 0.00%
Time (median): 41.804 ns ┊ GC (median): 0.00%
Time (mean ± σ): 42.091 ns ± 1.616 ns ┊ GC (mean ± σ): 0.00% ± 0.00%
▃ █▂ ▂
▂▂▅█▄▅█▁██▂▂▃▂▆█▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▂▂▁▂▁▂▁▁▂▂▁▂▂▁▂▂▂ ▃
40.2 ns Histogram: frequency by time 51.8 ns <
Memory estimate: 0 bytes, allocs estimate: 0.
julia> @benchmark linpol($q1,$q2,0.2)
BenchmarkTools.Trial: 10000 samples with 974 evaluations.
Range (min … max): 71.233 ns … 166.682 ns ┊ GC (min … max): 0.00% … 0.00%
Time (median): 72.098 ns ┊ GC (median): 0.00%
Time (mean ± σ): 72.533 ns ± 2.557 ns ┊ GC (mean ± σ): 0.00% ± 0.00%
▂ ▇ █ ▅ ▃ ▅ ▁ ▂
███▁█▅█▅█▅▄█▇█▇█▇▇▆▆▆▆▇▆▆▆▇▇▅▆▅▅▅▄▅▄▃▅▁▄▁▁▁▄▁▄▁▁▁▁▃▃▁█▃█▇▁▅▇ █
71.2 ns Histogram: log(frequency) by time 82.7 ns <
Memory estimate: 0 bytes, allocs estimate: 0. I'm not sure why |
Did you push this commit? |
Oh, I was forgetting. 😂 |
Co-authored-by: Seth Axen <seth.axen@gmail.com>
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.
LGTM! I'll approve, but before merging, can you merge master into this PR and increment the patch number? Then after merging you can register a new release.
This PR fixes #51.
Deprecate
linpol
As discussed in #51 (comment),
slerp
seems a more common name.slerp
now normalizes input quaternionsWe had the following choices, and I choose the first one:
linpol
)linpol
is deprecated for its name, and the originalslerp
for non-unit quaternions seems buggy, so I thought it's better to have the same behavior aslinpol
.DomainError
for non-unit quaternion.norm
field with Add isunit and change norm field to property #75, and checkingexp(log(q1)*(1-t) + log(q2)*t)
for any quaternions.The returned
Quaternion
hasnorm
flagtrue
.The flag will be removed with #75, but seems better to fix the flag for now.
TODO: