stephane-caron / pink

Python inverse kinematics based on Pinocchio
Apache License 2.0
246 stars 15 forks source link

Use a simplified formulation of the relative task #82

Closed ymontmarin closed 6 months ago

ymontmarin commented 6 months ago

This PR use a simpler formulation of the relative task, discarding some matrix multiplication in the Jacobian computation. It follows the discussion in: https://github.com/stephane-caron/pink/pull/80

It also changes the cost setting, to match the typing signature of the function and add the calculus of the jacobian in the description.

stephane-caron commented 6 months ago

Thank you for this contribution :smiley:

We can expect the following test to fail: test_relative_jacobian (in tests/test_relative_frame_task.py) since you changed the sign of the Jacobian: you can simply put a minus in front of J_check, that should do it.

coveralls commented 6 months ago

Pull Request Test Coverage Report for Build 8483559586

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pink/tasks/relative_frame_task.py 9 21 42.86%
<!-- Total: 9 21 42.86% -->
Totals Coverage Status
Change from base Build 8481800866: -0.6%
Covered Lines: 1278
Relevant Lines: 1293

💛 - Coveralls
ymontmarin commented 6 months ago

@stephane-caron I don't get the last formatting error concerning type annotation. Could you help ?

stephane-caron commented 6 months ago

@stephane-caron I don't get the last formatting error concerning type annotation. Could you help ?

The previous workaround for this was:

        if isinstance(self.cost, np.ndarray):  # helps mypy
            self.cost[0:3] = position_cost

MyPy is stating that the type if inferred for self.cost is float | Sequence[float] | ndarray[Any, Any] | None, which is correct since that's what we told it in the base class Task. (With such a general type we cannot evaluate self.cost[0:3], say if e.g. the runtime type is float or None.)

Looking at it again we can do better by adding a more precise type annotation in the class itself:

class RelativeFrameTask(Task):
    cost: np.ndarray
    frame: str
    ...

I checked locally and it fixes the MyPy error, so I suggest we go for that one :point_up:

ymontmarin commented 6 months ago

@stephane-caron You can merge if you think its ready !