Allow higher order spline derivatives. - #176
Conversation
bokorn-bdaii
left a comment
bokorn-bdaii
left a comment
There was a problem hiding this comment.
Looks good. Can you add the derivative computation to the unit tests and add a docstring to the derivative function.
| linear_vel = self.spline_xyz.derivative()(t) | ||
| angular_vel = self.spline_so3( | ||
| t, 1 | ||
| def derivative(self, t: float, order: int = 1) -> Twist3: |
There was a problem hiding this comment.
Add some warning that this only works for order = [1,2]. Current system throws a value error for order != [0,1,2] but will fail on order 0 with a "ValueError: bad value to Twist constructor" error. We should check and throw errors for order != [1,2].
There was a problem hiding this comment.
Thanks for testing that
| t, order | ||
| ) # 1 is angular rate, 2 is angular acceleration | ||
| return Twist3(linear_vel, angular_vel) | ||
| return Twist3(linear, angular) |
There was a problem hiding this comment.
Careful with semantics —Twist3 represents finite displacement (pose logarithm) rather than instantaneous velocity. Passing (v,ω) directly makes methods like .exp() implicitly bake in Δt=1 s. We should scale by dt or use a dedicated velocity class/structure.
This will work but the usage doesn't match with the documentation for the class. Perhaps need a VelocityTwist3 class.
Are you simply looking for a container to hold (v,ω) like a ROS Twist message?
petercorke
left a comment
petercorke
left a comment
There was a problem hiding this comment.
This method has no docstring and no test. The whole spline module is very light on docstrings, and the docstrings don't conform to the reST standard used through the rest of spatialmath. They are inconsistent 'google style' docstrings.
Agree with the other reviewer that there should be check on order.
Some methods have zero test coverage.
This is a useful functionality for the package but there's a bit of tech debt.
|
@jbarry-bdai this PR is almost trivial and has no risk. But the spline implementation needs work on test coverage and docstring formatting. I'm also surprised at the return value being a Not sure how to resolve this if Mark has left. I'd recommend approving it, but we should start a tech debt list somewhere. In other packages I've using Issues tagged 'tech-debt'. |
3 participants
Calculate higher order spline derivatives for the InterpSplineSE3 class.