-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add RotationGenerator
and its subtypes
#203
Add RotationGenerator
and its subtypes
#203
Conversation
Codecov Report
@@ Coverage Diff @@
## master #203 +/- ##
==========================================
+ Coverage 86.74% 87.57% +0.83%
==========================================
Files 14 15 +1
Lines 1403 1529 +126
==========================================
+ Hits 1217 1339 +122
- Misses 186 190 +4
Continue to review full report at Codecov.
|
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.
Oh cool. :)
I would still avoid the term "infinitesimal rotation". Julia can represent infinitesimal rotations already, but it's not these. Here's one, with dual numbers:
using DualNumbers
infinitesimal_rotation = [
1.0 Dual(0.0, 0.5);
Dual(0.0, -0.5) 1.0
]
What you have in this PR is a rotation generator, so if we aren't going to refer to the matrix structure then we may as well call it by its real name. Note the generator for the above an infinitesimal generator:
infinitesimal_generator = [
0.0 Dual(0.0, 0.5);
Dual(0.0, -0.5) 0.0
]
It's easy to figure this out using just exp(ϵ) ≈ I + ϵ
.
What doesn't quite work out-of-the-box in Julia is exp
and log
with Dual matrices, but mathematically the following should hold:
exp(infinitesimal_generator) == infinitesimal_rotation
Hopefully with your work here we'll be able to get all these relationships working even with autodifferentiation on the parameters of the generator! :)
Ah, I didn't quite understand the meaning of inifinitesimal rotation.
Yeah, I'll try to do that! 😄 |
Yes - I think that is perfect :) |
InfinitesimalRotation
and its subtypesRotationGenerator
and its subtypes
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.
Awesome.
I had questions about the method defined for promote_op
(and perhaps whatever you are trying to acheive there could be tested, too?) but otherwise this looks good to me.
This PR fills a massive gap, and I'm sure the community will appreciate it. Thanks @hyrodium!
src/rotation_generator.jl
Outdated
# Useful for converting arrays of rotations to another rotation eltype, for instance. | ||
# Only works because parameters of all the rotations are of a similar form | ||
# Would need to be more sophisticated if we have arbitrary dimensions, etc | ||
@inline function Base.promote_op(::Type{R1}, ::Type{R2}) where {R1 <: RotationGenerator, R2 <: RotationGenerator} |
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.
Is this right? The operator is not defined. Did you mean promote_rule
?
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.
Ah, I was just referring to src/core_types.jl
.
0ccc876#diff-55fa94d9eb6d180ed431c8aae0ad29c7100a0ad88cbd35dfc81239882a614e8dR39
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.
I've tried renaming promote_op
-> promote_rule
and isleaftype
-> isconcretetype
, but it doesn't work well here.
These seems unnecessary, so I've dropped both promote_op
definitions from src/core_types.jl
and src/rotation_generator.jl
.
This PR solves #30, #31,
#126 and #128.(2021/11/20 EDIT: quaternion conversions will be supported in the other PR.)