Check if internal J_cols definition is necessary

stack-of-tasks / pinocchio

A fast and flexible implementation of Rigid Body Dynamics algorithms and their analytical derivatives

http://stack-of-tasks.github.io/pinocchio/

BSD 2-Clause "Simplified" License

1.7k stars 366 forks source link

Check if internal J_cols definition is necessary #2173

Closed stephane-caron closed 4 months ago

stephane-caron commented 4 months ago

See https://github.com/stack-of-tasks/pinocchio/issues/2171

Checking if removing J_cols here breaks a unit test. There are two potential outcomes:

It is necessary and all tests pass, meaning we have an opportunity to add one :wink:
It is not necessary, all tests pass, we can double check and merge.

hrp2-14 commented 4 months ago

Hi ! This project doesn't usually accept pull requests on the main branch. If this wasn't intentionnal, you can change the base branch of this PR to devel (No need to close it for that). Best, a bot.

jcarpent commented 4 months ago

The quantity J_cols is exploited in the remainings sweeps of the computMinv of the algorithm.

stephane-caron commented 4 months ago

Ah OK, I hadn't noticed this is a static function. (Woah, those are error-prone.) Sorry for the noise.

I second stated goal of this PR was to check whether this removal breaks any unit test, which I then did locally, and it does break test-cpp-aba :ok_hand: