[Core] Fix Transform::log() and SpatialMotionVector::exp() #562
Conversation
prashanthr05
commented
Aug 25, 2019
prashanthr05
commented
- Adds left Jacobian and left Jacobian inverse of SO(3) to Rotation
- Fixes Transform::log()
- Fixes SpatialMotionVector::exp()
prashanthr05
changed the title
prashanthr05
changed the title
prashanthr05
changed the title
- Adds left Jacobian and left Jacobian inverse of SO(3) to Rotation - Fixes Transform::log() - Fixes SpatialMotionVector::exp()
|
@traversaro it'd be great if you could review this PR. |
|
|
||
| Transform t_v = v.exp(); | ||
|
|
||
| ASSERT_EQUAL_TRANSFORM_TOL(t_posrot, t_v, 0.01); |
There was a problem hiding this comment.
This test is not testing the linear part?
There was a problem hiding this comment.
Computationally, it is (meaning, the Jacobian is computed underneath and is multiplied with the linear part, and the computation does not crash). Quantitatively, no. Since the linear part is set to all zeros.
There was a problem hiding this comment.
I see, we are not checking if the function is correct, but at least we are checking stuff like that memory is correct with Valgrind, etc etc.
|
I am bit puzzled about that. In theory changing the behavior of a public function would call for creating a new function and deprecate the old one, but in practice the behavior of those function was not documented, so I think it is fair to consider the "wrong" behavior for the linear part as a bug. |
I do not fully comprehend this statement. You mean since some macro related to make_deprecated was not called since we made changes directly on the public API of Transform or SpatialMotionVector? |
I will explain to you in person. In a nutshell, in general you cannot change the behaviour of a public function, i.e. if you have a function |
Ah, okay. I understand and I agree. |
