Add support for higher dimensional rotation #219

hyrodium · 2022-01-16T05:37:16Z

This PR fixes #215.

RotMatrix{N} constructor
RotMatrixGenerator{N} constructor
exp function RotMatrixGenerator{N} to RotMatrix{N}
log function RotMatrix{N} to RotMatrixGenerator{N}
isrotationgenerator function
~~rotation_angle~~
rand method
documentation

hyrodium · 2022-01-16T06:12:38Z

The CI log says:

Testing isrotation: Error During Test at /home/runner/work/Rotations.jl/Rotations.jl/test/rotation_tests.jl:439
  Test threw exception
  Expression: isrotation(I(1))
  MethodError: objects of type UniformScaling{Bool} are not callable
  Stacktrace:
   [1] macro expansion at /home/runner/work/Rotations.jl/Rotations.jl/test/rotation_tests.jl:439 [inlined]
   [2] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]
   [3] macro expansion at /home/runner/work/Rotations.jl/Rotations.jl/test/rotation_tests.jl:407 [inlined]
   [4] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]
   [5] top-level scope at /home/runner/work/Rotations.jl/Rotations.jl/test/rotation_tests.jl:89

Like JuliaArrays/StaticArrays.jl#985, we can drop support under Julia v1.6.

codecov · 2022-01-28T23:09:34Z

Codecov Report

Merging #219 (9294916) into master (4f76c5a) will increase coverage by 0.41%.
The diff coverage is 98.82%.

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
+ Coverage   88.14%   88.56%   +0.41%     
==========================================
  Files          15       16       +1     
  Lines        1603     1626      +23     
==========================================
+ Hits         1413     1440      +27     
+ Misses        190      186       -4

Impacted Files	Coverage Δ
src/euler_types.jl	`92.25% <ø> (-0.18%)`	⬇️
src/mrps.jl	`34.93% <ø> (-0.88%)`	⬇️
src/rodrigues_params.jl	`98.64% <ø> (-0.04%)`	⬇️
src/unitquaternion.jl	`96.36% <ø> (-0.04%)`	⬇️
src/logexp.jl	`93.33% <93.33%> (+11.51%)`	⬆️
src/core_types.jl	`94.81% <100.00%> (+0.65%)`	⬆️
src/rand.jl	`100.00% <100.00%> (ø)`
src/rotation_generator.jl	`96.35% <100.00%> (+0.44%)`	⬆️

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 4f76c5a...9294916. Read the comment docs.

c42f

This looks great to me. Should we have isrotationgenerator(r, tol) implemented and use a small relative tolerance by default as in the algorithm for Base.≈?

src/rotation_generator.jl

Co-authored-by: Chris Foster <[email protected]>

c42f

LGTM

c42f · 2022-02-04T04:12:48Z

src/rotation_generator.jl

+    if indsm != indsn
+        return false
+    end
+    for i in indsn, j in i:last(indsn)


i:last(indsn)

Oh that's subtle. But I think it's correct given the indsm != indsn check.

Should we replace the line with this?

for i in indsm, j in i:last(indsn)

In the next line, we have the following script, so it seems okay to assume that both indices have the same axes.

if r[i,j] != -r[j,i]

Oh! now that you mention it, I'd somehow read that as i:last(indsm) already...

I think it's fine/good to leave this alone and not deviate from ishermitian() more than necessary.

hyrodium · 2022-02-04T13:08:25Z

Thanks for the review! I have added a TODO list in the description.

hyrodium · 2022-02-12T08:25:01Z

I was misunderstanding the rotation angle of highter dimensional rotation, and I have reverted the rotation_angle for highter dimensional rotation.
I thought a rotation have one rotation angle, but that was incorrect; in general, a n-dimensional rotation has n÷2 rotation angles.
See https://cs.brynmawr.edu/~dxu/206-2550-2.pdf for more information.

hyrodium · 2022-02-13T14:07:28Z

I have added documentation, and I think this is ready to merge.

c42f

Still looks good to me. I noticed a few minor things which could be cleaned up.

Codecov noticed a few new lines without tests - would be good to fix those too.

docs/src/reference.md

src/logexp.jl

c42f · 2022-02-15T08:50:03Z

src/rand.jl

@@ -0,0 +1,70 @@
+function Random.rand(rng::AbstractRNG, ::Random.SamplerType{R}) where R <: Union{<:Rotation{2},<:RotMatrix{2}}
+    # Union{<:Rotation{2},<:RotMatrix{2}} seems able to be replaced with Rotation{2},
+    # but it avoids the method for general dimensional RotMatrix.


I don't quite understand this comment

We can produce random RotMatrix{2} instance in two ways:

Generate random angle in [0,2π] and create RotMatrix{2} instance.

Generate random matrix from randn(2,2) and convert it to RotMatrix{2} with nearest_rotation.

The first one is much effective in terms of performance, and the last one can be generalized to high dimensional rotations.

The type R <: Union{<:Rotation{2},<:RotMatrix{2}} in the first line has been changed from the original R <: Rotation{2} to be calculated with uniform distribution on [0,2π] when calling rand(::RotMatrix{2}). (Note that RotMatrix{2}<:Rotation{2}, and Union{<:Rotation{2},<:RotMatrix{2}} is almost equivalent to Rotation{2})
I was trying to comment on the reason for this change, but maybe I could not make my English understood... 😵‍💫

Or, is there a better to make rand(RotMatrix{2}) select a method based on a uniform distribution of [0,2π] without using Union?

Oh, I think I see. So the point here is that including <:RotMatrix{2} makes this method of rand more specific than the method for general RotMatrix?

In that case I'd word this as something like

# We use the awkward Union{<:Rotation{2},<:RotMatrix{2}} here rather than `Rotation{2}` # to make this efficient 2D method more specific than the method for general `RotMatrix`

Your English is quite good! It's hard to express this tricky technical point succinctly.

Thanks for the suggestion! I've updated the comments that way.

src/rand.jl

Co-authored-by: Chris Foster <[email protected]>

hyrodium added 7 commits January 16, 2022 13:51

update nearest_rotation tests for 4-dimensional matrix

246dce6

update indents in rotation_tests.jl

7729009

update isrotation tests

390a6ff

updatge RotMatrix constructor

95544b2

update isrotation methods

4410180

add isrotationgenerator

387a9bd

add tests for isrotationgenerator

dac507d

hyrodium mentioned this pull request Jan 21, 2022

Drop support under Julia v1.6 #220

Merged

hyrodium closed this Jan 28, 2022

hyrodium reopened this Jan 28, 2022

hyrodium added 2 commits January 30, 2022 21:34

cleanup logexp.jl and add support for general dimensions

7896d0f

Merge branch 'master' into feature/higher_diemsions

49d712c

c42f reviewed Feb 3, 2022

View reviewed changes

src/rotation_generator.jl Outdated Show resolved Hide resolved

src/rotation_generator.jl Outdated Show resolved Hide resolved

src/rotation_generator.jl Outdated Show resolved Hide resolved

hyrodium and others added 4 commits February 3, 2022 12:54

Update src/rotation_generator.jl

e4f82ca

Co-authored-by: Chris Foster <[email protected]>

update around isrotationgenerator

bb20a74

update tests for general dimensional rotation generator

5b809ca

update general dimensional log(::RotMatrix{N})

f6f9ffa

hyrodium force-pushed the feature/higher_diemsions branch from b4e6a7a to f6f9ffa Compare February 3, 2022 14:47

c42f approved these changes Feb 4, 2022

View reviewed changes

update log(::RotMatix{N}) for Julia 1.6

d5191d0

hyrodium added 2 commits February 5, 2022 11:33

add tests for general dimensional rotation_angle

c445c8e

add general dimensional rotation_angle

d0a2008

revert general dimensional rotation_angle

3e08e9e

hyrodium force-pushed the feature/higher_diemsions branch from 71115de to 3e08e9e Compare February 12, 2022 09:22

hyrodium added 2 commits February 12, 2022 18:42

add rand method for higher dimensional rotation

fe39f45

add tests for higher dimensional RotMatrix

f93e470

hyrodium added 8 commits February 12, 2022 22:04

add rand.jl

8661d9b

update multiple dispatch of Random.rand for lower dimensional RotMatrix

a55824c

update comments

7be79fc

fix rng in rand(::RotMatrix{N})

abd694a

move rand methods to rand.jl

208f352

fix typos

d2bd13c

add documentation for general dimensional rotations

bbc2d0b

update reference

8758573

hyrodium marked this pull request as ready for review February 13, 2022 14:06

c42f reviewed Feb 15, 2022

View reviewed changes

hyrodium and others added 5 commits February 15, 2022 18:32

Update docs/src/reference.md

bc6f918

Co-authored-by: Chris Foster <[email protected]>

Update src/logexp.jl

8590a6d

Co-authored-by: Chris Foster <[email protected]>

remove duplicated end

ea74b27

remove Base.log(R::Rotation{2}) etc. and update some comments

fef8c8c

update comments suggested by @c42f

9294916

hyrodium mentioned this pull request Feb 16, 2022

Remove rand(::RotXY) method etc. #222

Open

hyrodium merged commit 345d0d9 into JuliaGeometry:master Feb 17, 2022

ferrolho mentioned this pull request May 9, 2023

BLAS.set_num_threads(1) drastically improves dynamics! performance for Atlas benchmark JuliaRobotics/RigidBodyDynamics.jl#500

Open

Add support for higher dimensional rotation #219

Add support for higher dimensional rotation #219

Uh oh!

Conversation

hyrodium commented Jan 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hyrodium commented Jan 16, 2022

Uh oh!

codecov bot commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

c42f left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

c42f left a comment

Choose a reason for hiding this comment

Uh oh!

c42f Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

hyrodium Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

c42f Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

hyrodium commented Feb 4, 2022

Uh oh!

hyrodium commented Feb 12, 2022

Uh oh!

hyrodium commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c42f left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

c42f Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

hyrodium Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c42f Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

hyrodium Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hyrodium commented Jan 16, 2022 •

edited

Loading

codecov bot commented Jan 28, 2022 •

edited

Loading

hyrodium commented Feb 13, 2022 •

edited

Loading

hyrodium Feb 15, 2022 •

edited

Loading