Correct `UnitQuaternion` with `Quaternions.jl` #175

hyrodium · 2021-10-29T09:26:59Z

This PR branch is based on #173, so please merge the PR first.

Done

Add dependency on Quaternions.jl
- UnitQuaternion scalar division #140 (comment)
- Add dependency on Quaternions.jl? #168
Remove some incorrect methods
- conj(::UnitQuaternion)
- -(::UnitQuaternion)
- *(::Real, ::UnitQuaternion)
- LinearAlgebra.norm(::Real, ::UnitQuaternion)
- LinearAlgebra.normalize(::Real, ::UnitQuaternion)
- (and etc.)
Add type conversion
- UnitQuaterinon <-> Quaternion
Update Rotations.pure_quaternion
- Rename to _pure_quaternion because this is "private" method.
- The return value type is now Quaternion. (previously, UnitQuaternion)

WIP, not in this PR

UnitQuaternoin{T} sitll have fields w::T, x::T, y::T and z::T, not q::Quaternion{T}.
- I'd like to keep this PR small, so this filed will be changed in another PR.
The name of unit quaternion type is still UnitQuaternion.
- This will be changed to QuatRotation in the future.
The return value type of _pure_quaternion is Quaternion.
- This should be InfinitesimalQuatRotation <: InfinitesimalRotation in the future.

codecov · 2021-10-29T09:48:46Z

Codecov Report

Merging #175 (de9cb6c) into master (d34316c) will decrease coverage by 0.22%.
The diff coverage is 68.00%.

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
- Coverage   86.33%   86.11%   -0.23%     
==========================================
  Files          14       14              
  Lines        1325     1340      +15     
==========================================
+ Hits         1144     1154      +10     
- Misses        181      186       +5

Impacted Files	Coverage Δ
src/mrps.jl	`33.56% <ø> (-0.47%)`	⬇️
src/rodrigues_params.jl	`98.61% <ø> (-0.02%)`	⬇️
src/core_types.jl	`90.22% <50.00%> (-0.46%)`	⬇️
src/unitquaternion.jl	`94.05% <71.42%> (-2.13%)`	⬇️
src/principal_value.jl	`100.00% <0.00%> (+4.16%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d34316c...de9cb6c. Read the comment docs.

hyrodium · 2021-10-29T10:00:35Z

Please ignore the coverage changes. This will be fixed when changing the field of UnitQuaternion.

hyrodium · 2021-10-29T10:23:34Z

The operators for UnitQuaternion such as conj, * and - were bugs, but some users might think that was a feature.
Is it okay to release these changes in patch version(v1.0.4)? Or, should we release minor version(v1.1.0)? (cf. #176)

hyrodium · 2021-10-29T13:34:16Z

I haven't added the [compat] table in Project.toml for Quaternions because I'd like to check that CompatHeler works fine. (#145)

andyferris

Nice @hyrodium.

I have no idea whether this is a bugfix or a breaking change. It's not exactly a new feature. It's possible that someone will rely on the current functionality.

andyferris · 2021-10-31T05:40:43Z

src/unitquaternion.jl

 vecnorm(q::UnitQuaternion) = sqrt(q.x^2 + q.y^2 + q.z^2)
+vecnorm(q::Quaternion) = sqrt(q.v1^2 + q.v2^2 + q.v3^2)


We shouldn't be defining things on behalf of Quaternions.jl.

Thanks, that was my mistake. I've removed the line.

andyferris · 2021-10-31T05:42:24Z

src/unitquaternion.jl


 # Norms
-LinearAlgebra.norm(q::UnitQuaternion) = sqrt(q.w^2 + q.x^2 + q.y^2 + q.z^2)
 vecnorm(q::UnitQuaternion) = sqrt(q.x^2 + q.y^2 + q.z^2)


The vecnorm must be defined in terms of the matrix. Which is orthogonal, so that's easy. Should we just define this instead?

vecnorm(::Rotation{N, T}) where {N, T} = convert(T, N)

The vecnorm function was to calculate the norm of the imaginary part of a quaternion, and it was used only in _log_as_quat. So, I've removed the function. (09e0313)

andyferris · 2021-10-31T05:46:45Z

src/unitquaternion.jl

+(\)(q1::UnitQuaternion, q2::UnitQuaternion) = inv(q1)*q2  # Equivalent to inv(q1)*q2
+(/)(q1::UnitQuaternion, q2::UnitQuaternion) = q1*inv(q2)  # Equivalent to q1*inv(q2)

-
-(\)(q1::UnitQuaternion, q2::UnitQuaternion) = conj(q1)*q2  # Equivalent to inv(q1)*q2
-(/)(q1::UnitQuaternion, q2::UnitQuaternion) = q1*conj(q2)  # Equivalent to q1*inv(q2)
-
-(\)(q::UnitQuaternion, r::SVector{3}) = conj(q)*r          # Equivalent to inv(q)*r
+(\)(q::UnitQuaternion, r::SVector{3}) = inv(q)*r          # Equivalent to inv(q)*r


Aren't these defined for all rotations? (I.e. can they be deleted instead?)

With original definition:

julia> @benchmark q\SVector(1,2,3) # q isa UnitQuaternion BenchmarkTools.Trial: 10000 samples with 996 evaluations. Range (min … max): 25.933 ns … 1.724 μs ┊ GC (min … max): 0.00% … 97.61% Time (median): 26.546 ns ┊ GC (median): 0.00% Time (mean ± σ): 28.727 ns ± 42.051 ns ┊ GC (mean ± σ): 3.85% ± 2.59% ▂▆█▇▄▂ ▁▂▂▂▂▂▁▁ ▁ ██████▇▆▇▆▆▆▅▅▅▅▃▅▅▆▆▆▅▅▅▆▇▇▇██████████▇▆▆▆▅▄▄▆▆▆▅▅▆▅▄▅▄▄▄▆ █ 25.9 ns Histogram: log(frequency) by time 38.8 ns < Memory estimate: 32 bytes, allocs estimate: 1.

If Removing the definition:

julia> @benchmark q\SVector(1,2,3) # q isa UnitQuaternion BenchmarkTools.Trial: 10000 samples with 780 evaluations. Range (min … max): 160.083 ns … 2.153 μs ┊ GC (min … max): 0.00% … 92.03% Time (median): 164.683 ns ┊ GC (median): 0.00% Time (mean ± σ): 167.640 ns ± 44.395 ns ┊ GC (mean ± σ): 0.59% ± 2.06% ▂▃█▁▁ ▂▂▁▁▃▇█████▇▇▇▄▄▃▃▃▃▃▃▃▄▄▄▄▃▃▃▃▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂ ▃ 160 ns Histogram: frequency by time 186 ns < Memory estimate: 32 bytes, allocs estimate: 1.

This performance deterioration is because (\)(::Rotation, ::SVector{3}) is not defined.

So, we need to update the \ definition here. With this update, we'll get more performance with other rotation types.

julia> using Rotations, StaticArrays, BenchmarkTools julia> r = RotX(2.3) 3×3 RotX{Float64} with indices SOneTo(3)×SOneTo(3)(2.3): 1.0 0.0 0.0 0.0 -0.666276 -0.745705 0.0 0.745705 -0.666276 julia> @benchmark r\SVector(1,2,3) BenchmarkTools.Trial: 10000 samples with 861 evaluations. Range (min … max): 138.636 ns … 1.605 μs ┊ GC (min … max): 0.00% … 90.34% Time (median): 154.602 ns ┊ GC (median): 0.00% Time (mean ± σ): 156.485 ns ± 31.911 ns ┊ GC (mean ± σ): 0.45% ± 2.01% ▂▇▇▅█▆▄▃▄ ▂▁▁▁▁▂▁▂▂▁▂▂▂▂▂▂▂▂▃▂▂▃▃▄▄▄▆█████████▇█▄▃▄▃▄▃▄▄▄▄▅▅▅▄▃▃▂▂▂▂▂▂ ▄ 139 ns Histogram: frequency by time 168 ns < Memory estimate: 32 bytes, allocs estimate: 1. julia> Base.:\(r::Rotation, v::SVector{3}) = inv(r)*v julia> @benchmark r\SVector(1,2,3) BenchmarkTools.Trial: 10000 samples with 994 evaluations. Range (min … max): 29.926 ns … 1.726 μs ┊ GC (min … max): 0.00% … 97.98% Time (median): 30.742 ns ┊ GC (median): 0.00% Time (mean ± σ): 32.839 ns ± 44.139 ns ┊ GC (mean ± σ): 3.54% ± 2.59% ▃███▇▅▃▂ ▁▂▂▂▂▂▂▂▁ ▂ ██████████▇▇▇▆▇█▆▆▆▆▅▆▇▆▆▇▇▇███████████▇▆▆▅▅▅▆▅▅▆▅▄▄▄▅▅▅▅▅▆ █ 29.9 ns Histogram: log(frequency) by time 43.2 ns < Memory estimate: 32 bytes, allocs estimate: 1.

I've added the method. (9647c84)

hyrodium · 2021-11-04T12:09:10Z

I have no idea whether this is a bugfix or a breaking change. It's not exactly a new feature. It's possible that someone will rely on the current functionality.

Should we release v2.0.0? I'd like to think that was a bug...

hyrodium · 2021-11-08T13:43:54Z

@andyferris
Can I merge this PR? I've committed after your approval.

andyferris

@hyrodium yes feel free to merge

* remove incorrect log(::Quaternion) * add method for log(::Rotation) and return SMatrix * fix log method for Rotation{2} * update tests for logarithm function * add dependency on Quaternions.jl * update exports for deprecated types * update around pure_quaternion * remove incorrect LinearAlgebra.norm * remove incorrect *(::Real, UnitQuaternion) * remove incorrect conj(Quaternion) * remove incorrect -(::UnitQuaternion) * update tests around norm related functions * update src and test around norm related functions * remove unnecessary import from Base * remove exp method for Quaternion * update type conversions UnitQuaternion <-> Quaternion * fix tests for Julia under v1.5 * remove unnecessary lines * remove vecnorm(q::UnitQuaternion) function * update comments * add methods for Base.:\(r::Rotation, v) * update UnitQuaternion constructor

dehann · 2021-11-22T04:10:54Z

Kindly request in future to please add deprecation cycles as per semver for breaking changes. You can easily overload Base.get/setproperty with a deprecation warning. In our case I had to dig to find out why quat.w no longer worked, which was replaced by quat.q.s or .v1/v2/v3. Here is a pattern example we use in such cases:
https://github.com/JuliaRobotics/IncrementalInference.jl/wiki/Coding-Templates#deprecate-struct-field

hyrodium · 2021-11-22T04:54:14Z

@dehann
Sorry for the inconvenience. I thought the fields are just internal parameters because we provide Rotations.params method.
This function is not exported, but will be maintained.

And related things, I think we need to distinguish among exported method, non-exported method and internal use method.

exported method
- e.g.RotXYZ, nearest_rotation
- These methods should be maintained.
non-exported method
- e.g. Rotations.params, Rotations.jacobian
- These methods also should be maintained.
- The reason for non-exported is just avoiding name conflict.
internal use method
- e.g. _puer_quaternion
- These functions are regarded as an internal method, and may be changed without breaking release.
- In Rotations.jl, many functions should be renamed with underscore _. (e.g. vee, lmult)
  - I'm not sure if such renamings would be a breaking change.
  - See blue guide for more information.
- Also sorry, for the inconvenience(pure_quaternion not defined RoboticExplorationLab/RobotDynamics.jl#12)

bzinberg · 2021-11-23T01:29:41Z

@dehann, thanks for your insights on this! The breakage now has its own issue, #208, let's move discussion there.

hyrodium added 16 commits October 28, 2021 12:45

remove incorrect log(::Quaternion)

3ec59bc

add method for log(::Rotation) and return SMatrix

630509b

fix log method for Rotation{2}

e0dc787

update tests for logarithm function

8324da6

add dependency on Quaternions.jl

12fa6df

update exports for deprecated types

9f49e46

update around pure_quaternion

4300706

remove incorrect LinearAlgebra.norm

49f4bd4

remove incorrect *(::Real, UnitQuaternion)

67f4f1a

remove incorrect conj(Quaternion)

c78ca5a

remove incorrect -(::UnitQuaternion)

2a5b7a6

update tests around norm related functions

ff13068

update src and test around norm related functions

7613de0

remove unnecessary import from Base

983286b

remove exp method for Quaternion

4b630d0

update type conversions UnitQuaternion <-> Quaternion

666a4cb

hyrodium requested a review from andyferris October 29, 2021 09:26

hyrodium changed the title ~~Correct quaternion with Quaternions.jl~~ Correct UnitQuaternion with Quaternions.jl Oct 29, 2021

fix tests for Julia under v1.5

ca48fc5

andyferris approved these changes Oct 31, 2021

View reviewed changes

hyrodium added 5 commits October 31, 2021 22:40

remove unnecessary lines

ea6f600

Merge branch 'master' into feature/correct_quaternion

934cf9b

remove vecnorm(q::UnitQuaternion) function

09e0313

update comments

cfb4b5a

add methods for Base.:\(r::Rotation, v)

9647c84

hyrodium requested a review from andyferris November 1, 2021 13:23

This was referenced Nov 2, 2021

Add dependency on Quaternions.jl? #168

Closed

Incorrect norm(::RodriguesParam) #187

Closed

hyrodium mentioned this pull request Nov 6, 2021

UnitQuaternion quaternion number equality conflicts with rotation matrix equality #189

Closed

update UnitQuaternion constructor

de9cb6c

hyrodium mentioned this pull request Nov 8, 2021

Update fields of UnitQuaternion #191

Merged

andyferris approved these changes Nov 10, 2021

View reviewed changes

hyrodium merged commit 39a2f3c into JuliaGeometry:master Nov 10, 2021

hyrodium mentioned this pull request Nov 10, 2021

UnitQuaternion scalar division #140

Closed

bzinberg mentioned this pull request Nov 12, 2021

Breaking release #198

Closed

hyrodium mentioned this pull request Nov 17, 2021

Bump version to v1.1.0 #202

Merged

aabouman mentioned this pull request Nov 17, 2021

pure_quaternion not defined RoboticExplorationLab/RobotDynamics.jl#12

Open

dehann mentioned this pull request Nov 22, 2021

fix missing deprecation UnitQuaterion JuliaRobotics/Caesar.jl#781

Merged

This was referenced Nov 22, 2021

Rename internal use functions #204

Open

Deprecated UnitQuaternion doesn't work #208

Closed

hyrodium mentioned this pull request Apr 9, 2022

adding conversion functionality JuliaGeometry/Quaternions.jl#24

Closed

hyrodium mentioned this pull request Apr 26, 2022

Algebraic quaternions vs geometric quaternions JuliaGeometry/Quaternions.jl#10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct `UnitQuaternion` with `Quaternions.jl` #175

Correct `UnitQuaternion` with `Quaternions.jl` #175

hyrodium commented Oct 29, 2021 •

edited

Loading

codecov bot commented Oct 29, 2021 •

edited

Loading

hyrodium commented Oct 29, 2021

hyrodium commented Oct 29, 2021 •

edited

Loading

hyrodium commented Oct 29, 2021 •

edited

Loading

andyferris left a comment

andyferris Oct 31, 2021

hyrodium Oct 31, 2021

andyferris Oct 31, 2021 •

edited

Loading

hyrodium Oct 31, 2021

andyferris Oct 31, 2021

hyrodium Oct 31, 2021 •

edited

Loading

hyrodium Oct 31, 2021

hyrodium Oct 31, 2021

hyrodium commented Nov 4, 2021

hyrodium commented Nov 8, 2021

andyferris left a comment

dehann commented Nov 22, 2021

hyrodium commented Nov 22, 2021 •

edited

Loading

bzinberg commented Nov 23, 2021

		vecnorm(q::UnitQuaternion) = sqrt(q.x^2 + q.y^2 + q.z^2)
		vecnorm(q::Quaternion) = sqrt(q.v1^2 + q.v2^2 + q.v3^2)

Correct UnitQuaternion with Quaternions.jl #175

Correct UnitQuaternion with Quaternions.jl #175

Conversation

hyrodium commented Oct 29, 2021 • edited Loading

Done

WIP, not in this PR

codecov bot commented Oct 29, 2021 • edited Loading

Codecov Report

hyrodium commented Oct 29, 2021

hyrodium commented Oct 29, 2021 • edited Loading

hyrodium commented Oct 29, 2021 • edited Loading

andyferris left a comment

Choose a reason for hiding this comment

andyferris Oct 31, 2021

Choose a reason for hiding this comment

hyrodium Oct 31, 2021

Choose a reason for hiding this comment

andyferris Oct 31, 2021 • edited Loading

Choose a reason for hiding this comment

hyrodium Oct 31, 2021

Choose a reason for hiding this comment

andyferris Oct 31, 2021

Choose a reason for hiding this comment

hyrodium Oct 31, 2021 • edited Loading

Choose a reason for hiding this comment

hyrodium Oct 31, 2021

Choose a reason for hiding this comment

hyrodium Oct 31, 2021

Choose a reason for hiding this comment

hyrodium commented Nov 4, 2021

hyrodium commented Nov 8, 2021

andyferris left a comment

Choose a reason for hiding this comment

dehann commented Nov 22, 2021

hyrodium commented Nov 22, 2021 • edited Loading

bzinberg commented Nov 23, 2021

Correct `UnitQuaternion` with `Quaternions.jl` #175

Correct `UnitQuaternion` with `Quaternions.jl` #175

hyrodium commented Oct 29, 2021 •

edited

Loading

codecov bot commented Oct 29, 2021 •

edited

Loading

hyrodium commented Oct 29, 2021 •

edited

Loading

hyrodium commented Oct 29, 2021 •

edited

Loading

andyferris Oct 31, 2021 •

edited

Loading

hyrodium Oct 31, 2021 •

edited

Loading

hyrodium commented Nov 22, 2021 •

edited

Loading