Open trahflow opened 6 months ago
Thank you for the detailed issue description!
I have not implemented the differentiation (jacobian
, ∇rotate
, etc.) part, but your comment looks correct.
I will review the PR in a week!
This would probably also directly help with #129 (which I could maybe help with if I find time and confidence :upside_down_face: )
The method
jacobian(::Type{RotMatrix}, ::QuatRotation)
is supposed to implement the jacobian ofvec(RotMatrix(q))
with respect to the parameters ofq
, whereq isa QuatRotation
. The docstring says:Since
QuatRotation
can be constructed with or without normalization of the input quaternion, it is not entirely clear to me what this means. I think there are essentially two possibilities:vec(RotMatrix(q))
with respect to the parameters of a unit quaternion, i.e. the jacobian ofvec(RotMatrix(QuatRotation(p, false)))
, wherep
is a SVector{4} withnorm(p) == 1
.vec(RotMatrix(q))
with respect to the parameters of a general quaternion, i.e. the jacobian ofvec(RotMatrix(QuatRotation(p, true)))
, wherep
is a SVector{4} with arbitrary norm. In this case the jacobian should be the product of the jacobian of the above case, times the jacobian of the normalization operation.Now what
jacobian(::Type{output_param}, R::input_param)
tries to implement is apparently the second case, i.e. the jacobian including the normalization operation. However even though it includes the normalization operation, the implementation still assumes that the input quaternion is normalized. I think this is inconsistent (but probably even a bug?)There is a test for this method but it only tests for the case where the
QuatRotation
is constructed with an already normalized unit quaternion:If one would change line 3 st
q = rand(QuatRotation)
-->q = QuatRotation(rand(QuaternionF64), false)
, the test would actually fail.So in summary I think this method should either assume a unit quaternion and thus exclude the jacobian of the normalization operation, or assume a general (not necessarily unit-) quaternion and include the jacobian of the normalization operation.
One suggestion would be to add a
jacobian(::Type{RotMatrix}, ::Quaternion)
(note theQuaternion
instead ofQuatRotation
), which assumes a general quaternion and returns the jacobian including the normalization term. And change the behavior of the existingjacobian(::Type{RotMatrix}, ::QuatRotation)
to return the jacobian without the normalization term. ~I could provide a PR for that.~ I've proposed a PR: #291 which relaxes above test in said way and provides the suggested change (as a base for discussion).