laas / metapod

A template-based robot dynamics library
GNU Lesser General Public License v3.0
14 stars 10 forks source link

use Eigen abstract types instead of concrete types when possible #62

Open thomas-moulard opened 11 years ago

thomas-moulard commented 11 years ago

Dear all, I have the following code which is sub-optimal:

  robot_t::confVector torques;
  for (unsigned frameId = 1; frameId < animatedMesh_->numFrames () - 1;
   ++frameId)
{
  metapod::rnea<robot_t, true>::run
    (robot, q[frameId], dq[frameId], ddq[frameId]);
  metapod::getTorques (robot, torques);

  result.segment
    (frameId * robot_t::NBDOF, robot_t::NBDOF) = torques;
}

...the following version would avoid a copy but is not possible right now:

  metapod::getTorques (robot, result.segment
    (frameId * robot_t::NBDOF, robot_t::NBDOF));

Would it be possible to make sure that algorithms and functions take a EigenBase<T>, MatrixBase<T> or any other adequate abstract type instead of concrete type so that metapod can avoid enforcing its own matrix type confVector to everyone.

A side effect would be to let the user choose its own scalar type, matrix implementation, etc. which could lead to many potential optimization on the end-user side.

Thanks!

olivier-stasse commented 11 years ago

On 04/18/2013 07:16 AM, Thomas Moulard wrote:

Dear all, I have the following code which is sub-optimal:

| robott::confVector torques; for (unsigned frameId = 1; frameId < animatedMesh->numFrames () - 1; ++frameId) { metapod::rnea<robot_t, true>::run (robot, q[frameId], dq[frameId], ddq[frameId]); metapod::getTorques (robot, torques);

result.segment (frameId * robot_t::NBDOF, robot_t::NBDOF) = torques; } |

...the following version would avoid a copy but is not possible right now:

metapod::getTorques (robot, result.segment (frameId * robot_t::NBDOF, robot_t::NBDOF));

Would it be possible to make sure that algorithms and functions take a |EigenBase|, |MatrixBase| or any other adequate abstract type instead of concrete type so that metapod can avoid enforcing its own matrix type |confVector| to everyone.

I agree that this is a problem.

A side effect would be to let the user choose its own scalar type, matrix implementation, etc. which could lead to many potential optimization on the end-user side.

This is already a feature in the pile of TODO list for the scalar type.

Regarding the matrix implementation we could but I do not think it is a good idea. The Eigen matrix unlike a lot of other matrix implementation, performs some kind of formal computing which I believe to be quite important in the current performances. I currently have a bad experience with trying to make a generic matrix implementation (jrl-*).

More generally, the way the data are stored is still not fixed due to current tests of metapod on two differents architectures:

This may lead to dramatic changes between releases.

Finally due to other commitments, my current involvment in metapod is unfortunately limited...

However the confVector issue should be not too difficult to fix, I'll try to propose a fix.

Thanks!

— Reply to this email directly or view it on GitHub https://github.com/laas/metapod/issues/62.

thomas-moulard commented 11 years ago

Sorry, I was not clear enough.

You currently have:

myalgo(confVector&)

You should replace it by:

 template <typename T>
 myalgo(Eigen::MatrixBase<T>&)

...this would not impact the performances as the type will be resolved at compile time. In the worst case, you still can template by T and add a static assert making sure that T inherits from MatrixBase.

sbarthelemy commented 11 years ago

@thomas-moulard:

regarding the scalar type, the current plan is to template all the Spatial classes and the models ones by the Scalar type, à la Eigen.

Yes the current code involves copies and conversion to concrete Eigen types, which should be avoided. An alternative to your suggestion would be to use the new "Ref" classes that Eigen 3.2 (not released yet) introduces. I'd like to give it a try, and in any way fix these conversions, but have no ETA yet.

thomas-moulard commented 11 years ago

Yes, unfortunately, Ref are very new and I had some trouble with block operations with them. Also relying on bleeding edge version makes metapod a bit harder to use. But I agree with you, in a couple of months, ref may be the solution to a lot of problems :)

olivier-stasse commented 11 years ago

Dear all, I just proposed a version of metapod with a templated scalar type in topic/float-type-templated. This do not change too much the robot API. The benchmarks, robotbuilder and sample template have been update accordingly all the unitary test runs, and there is no impact on the performance as expected. Best, Olivier

olivier-stasse commented 11 years ago

I would like some comments/review before merging to the master (soon).

thomas-moulard commented 11 years ago

FYI Eigen 3.2.0 contains Ref. They allow to pass matrices independently of their real type (block, etc.) without requiring the code to be templated. It would be perfect to implement this feature with Ref(s).

Sorry I don't have time to do that myself though.