Skip to content
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

Breaking release #198

Closed
bzinberg opened this issue Nov 12, 2021 · 16 comments
Closed

Breaking release #198

bzinberg opened this issue Nov 12, 2021 · 16 comments

Comments

@bzinberg
Copy link

bzinberg commented Nov 12, 2021

In v1.0.3, the return type of log(::UnitQuaternion) is UnitQuaternion. In v1.0.4, it's SMatrix{3, 3}. I see from #175 (comment) that you already knew that. Yeah... I was relying on that type signature. Actually, what I was relying on was that exp(log(::UnitQuaternion)) has type UnitQuaternion.

@bzinberg
Copy link
Author

@hyrodium @andyferris, it seems like exp(log(::UnitQuaternion)) will indeed be a UnitQuaternion (or at least a Rotation{3}) once there is a special type for skew-symmetric matrices, right? So I think it's fair to say that v1.0.4 is a breaking release, and will hopefully be fixed soon?

@hyrodium
Copy link
Collaborator

Sorry for the inconvenience.
I'll work on this on a branch hotfixes/v1.0.5 and release v1.0.5 as soon as possible.

@hyrodium
Copy link
Collaborator

it seems like exp(log(::UnitQuaternion)) will indeed be a UnitQuaternion (or at least a Rotation{3})

The Lie algebra 𝔰𝔲(2) and 𝔰𝔬(3) are just 3-dimensional linear spaces.
So, we don't need many subtypes of InfinitesimalRotation unlike Rotation.

I think the easiest implementation will be like this:

abstract type InfinitesimalRotation{N,T} <: StaticMatrix{N,N,T} end

struct InfinitesimalRotationVec{T} <: InfinitesimalRotation{3,T}
   x::T
   y::T
   z::T
end

struct InfinitesimalAngle2d{T} <: InfinitesimalRotation{2,T}
    v::T
end

exp(v::InfinitesimalRotationVec) = RotationVec(v.x, v.y, v.z)
exp(v::InfinitesimalRotation2d) = Angle2d(v.v)

log(::Rotation{3}) isa InfinitesimalRotationVec
log(::Rotation{2}) isa InfinitesimalAngle2d

Does this make sense? (related: #190)

@bzinberg
Copy link
Author

@hyrodium

Sorry for the inconvenience. I'll work on this on a branch hotfixes/v1.0.5 and release v1.0.5 as soon as possible.

No worries, and not urgent from my side -- it was easy to fix in my package by adding a coercion to RotMatrix{3} in one place.

@bzinberg
Copy link
Author

exp(v::InfinitesimalRotationVec) = RotationVec(v.x, v.y, v.z)
exp(v::InfinitesimalRotation2d) = Angle2d(v.v)

log(::Rotation{3}) isa InfinitesimalRotationVec
log(::Rotation{2}) isa InfinitesimalAngle2d

Does this make sense?

Yeah, that makes sense to me. It might be slightly clearer to rename InfinitesimalRotationVec to something like SkewSymmetricMatrix3, with (a docstring and) field names that indicate how the fields correspond to entries of the matrix (perhaps a21, a31, a32). Then that type could have a conversion to SMatrix{3, 3}, and if LinearAlgebra later gets its own SkewSymmetric type, we can swap that in, per #171 (comment).

@andyferris
Copy link
Contributor

Yeah I too would name it for the matrix property (skew/antisymmetric). Hopefully eventually in LinearAlgebra we’ll have Orthogonal and Unitary and AntiHermitian one day, and we’ll be able to use that.

My understanding with the recent release the previous behaviour was a bug (the rotations here are “just” matrices and the previous behaviour did not respect that).

In hindsight it might have been easier for users to cope with the change if we had of made a major release, but now that the cat’s out of the bag I’m not so sure. When we have the new types that will warrant a minor release.

@hyrodium
Copy link
Collaborator

hyrodium commented Nov 13, 2021

Yeah, that makes sense to me. It might be slightly clearer to rename InfinitesimalRotationVec to something like SkewSymmetricMatrix3,

I think we can introduce two subtypes of InfinitesimalRotation{3}: InfinitesimalRotMatrix{3} and InfinitesimalRotationVec. InfinitesimalRotMatrix{N} will be a simple wrapper of SMatrix{N,N}, like RotMatrix{N}.

The name SkewSymmetricMatrix3 seems irrelevant from rotations, so the type should be defined in StaticArrays (or LinearAlgebra), I guess.

Then that type could have a conversion to SMatrix{3, 3}, and if LinearAlgebra later gets its own SkewSymmetric type, we can swap that in, per #171 (comment).

I agree with that.

My understanding with the recent release the previous behaviour was a bug (the rotations here are “just” matrices and the previous behaviour did not respect that).

I agree with that, and #173 introduced a new bug: exp(log(::UnitQuaternion)) isa Rotation is false...

@andyferris
Copy link
Contributor

I agree that names like OrthogonalMatrix and SkewSymmetricMatrix belong in parent packages. But until they do we may as well use the correct name, no? RotMatrix is more like SpecialOrthogonalMatrix which is a little long).

To be fair exp and log were never supported properly/correctly.

@hyrodium
Copy link
Collaborator

But until they do we may as well use the correct name, no?

Hmm, if they have StaticArrays.SkewSymmetricMatrix3 in the future, should we rename the type in Rotation? I thought that will make it confusing.

Is the length of the type name the only problem?

@bzinberg
Copy link
Author

think we can introduce two subtypes of InfinitesimalRotation{3}: InfinitesimalRotMatrix{3} and InfinitesimalRotationVec. InfinitesimalRotMatrix{N} will be a simple wrapper of SMatrix{N,N}, like RotMatrix{N}.

What would be the difference between those two types? Would they be exactly the same except for what return value they produce when passed into Base.exp?

The name SkewSymmetricMatrix3 seems irrelevant from rotations, so the type should be defined in StaticArrays (or LinearAlgebra), I guess.

The abstract Lie algebra so3 is canonically identified with the set of 3x3 skew-symmetric matrices. So, based on the discussion around #171 (comment), I was assuming it would be more in the current spirit of this package to call the type SkewSymmetricMatrix{3}. If you (the maintainers of the package) want to go with a more abstract name like InfinitesimalRotation{3} that doesn't commit to that identification, that's totally fine with me, and in fact is what I originally suggested 🙂 As for why I said SkewSymmetricMatrix3 rather than SkewSymmetricMatrix{3}, I was just trying to save work -- I assumed it would be easier to design a fixed-size skew-symmetric matrix abstraction rather than implementing and testing the parametric type SkewSymmetricMatrix{N} (which might be best saved for the LinearAlgebra maintainers, unless you're also them). If you are up for doing the parametric version, go for it.

@hyrodium
Copy link
Collaborator

What would be the difference between those two types? Would they be exactly the same except for what return value they produce when passed into Base.exp?

These are almost identical. Here's the difference:

  • InfinitesimalRotMatrix{N,T}: simple wrapper of SMatrix{N,N,T} (it also can support higher dimensions)
  • InfinitesimalRotationVec{T}: parameterized version of InfinitesimalRotMatrix{3,T}.
  • InfinitesimalAngle2d{T}: parameterized version of InfinitesimalRotMatrix{2,T}.
  • InfinitesimalRotation: their abstract supertype.

in fact is what I originally suggested 🙂

Thanks for the grreat naming 😁

If you are up for doing the parametric version, go for it.

Thanks, I'll try it.

@hyrodium
Copy link
Collaborator

I'm currently implementing infinitesimal rotation in #199.

Initially, I was planning to release v1.0.5 with as few new features as possible. However, I realized that incorporating InfinitesimalRotation would require more testing, and releasing it as a hotfix was not a good idea.
If exp(log(r::UnitQuaternion)) isa SMatrix makes problems, then I would like users to fix the version of Rotations.jl.

Thus, I think the next release will be v1.1.0 with new features:

  • Add nearest_rotation function.
  • Compatible with Quaternions.jl. (waiting for a review of Update fields of UnitQuaternion #191)
  • Support Lie algebra, InfinitesimalRotation.
  • Rename UnitQuaternion with QuatRotation and deprecate UnitQuaternion.

@hyrodium
Copy link
Collaborator

Update:
Adding InfinitesimalRotation may take time, so I'd like to release v1.1.0 for now.
The future releases will be:

  • v1.2.0: add support for infinitesimal rotations
  • v1.3.0: add differentiation with ChainRulesCore.jl

@c42f
Copy link
Member

c42f commented Nov 17, 2021

Awesome stuff! It's so great to see all the energy going into improving this package :-)

@bzinberg
Copy link
Author

Yeah, much thanks everyone especially @hyrodium!

@c42f @hyrodium, does ChainRulesCore.jl have a coordinate-free interface? Part of the benefit of working in the various different coordinate charts that Rotations.jl provides is that they allow one to avoid some of the numeric issues that come from singularities. I didn't read the docs deeply yet, but my assumption was that ChainRulesCore.jl is for maps from R^n to R^m (and maybe tensor products of those things) only. I am definitely interested in building an API for abstract manifolds, but that's a big project that I haven't yet seen be the focus for an AD package.

@hyrodium
Copy link
Collaborator

The original issue is now solved with #203.

julia> using Rotations

julia> q = QuatRotation(1., 0., 0., 0.)
3×3 QuatRotation{Float64} with indices SOneTo(3)×SOneTo(3)(Quaternion{Float64}(1.0, 0.0, 0.0, 0.0, true)):
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

julia> log(q)
3×3 RotationVecGenerator{Float64} with indices SOneTo(3)×SOneTo(3)(0.0, 0.0, 0.0):
  0.0  -0.0   0.0
  0.0   0.0  -0.0
 -0.0   0.0   0.0

julia> exp(log(q))
3×3 RotationVec{Float64} with indices SOneTo(3)×SOneTo(3)(0.0, 0.0, 0.0):
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

julia> exp(log(q)) isa Rotation
true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants