robotology / human-dynamics-estimation

Software repository for estimating human dynamics
BSD 3-Clause "New" or "Revised" License
81 stars 28 forks source link

[HDELibrary] Expose target error norms in Dynamical Inverse Kinematics #291

Closed prashanthr05 closed 2 years ago

prashanthr05 commented 2 years ago

This PR exposes the mean error norm computed for all the rotation and position targets in Dynamical IK.

P.S. my IDE editor automatically removed the blank spaces in the files. Let me know if these changes on the blank spaces need to be reverted.

cc @lrapetti

lrapetti commented 2 years ago

P.S. my IDE editor automatically removed the blank spaces in the files. Let me know if these changes on the blank spaces need to be reverted.

that's fine, I think it was due to the fact that the clang-format was not always used in the past.

prashanthr05 commented 2 years ago

Looks good.

Just one comment, I see the error norm is computed on the number of targets, but consider that a position error can have 1,2, or 3 components (xyz), and the rotation error can depend on 1 or all three axis, depending on how they are configured. I don't know if you need to consider this fact or if it is ok like this

Ah, then it is necessary to be more elaborate, to check for active axis/direction checks and handle the number of targets accordingly. If you agree, I will make the relevant changes.

lrapetti commented 2 years ago

Ah, then it is necessary to be more elaborate, to check for active axis/direction checks and handle the number of targets accordingly. If you agree, I will make the relevant changes.

I think it is mostly a matter of choice if to normalize the error on the number of targets or the number of components. Personally, I don't have a strong opinion on that, I just wanted to mention this aspect

prashanthr05 commented 2 years ago

I have made some changes in https://github.com/robotology/human-dynamics-estimation/pull/291/commits/0aa72e6b0b20f9efe639a75f6eed863a2d6ea270 in the error norm computation based on active axes.

lrapetti commented 2 years ago

I have made some changes in 0aa72e6 in the error norm computation based on active axes.

I checked it and it looks good, thanks!

lrapetti commented 2 years ago

@prashanthr05 do you think we can merge it?

prashanthr05 commented 2 years ago

@prashanthr05 do you think we can merge it?

Yes, thanks @lrapetti!!