Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using the Eigen library for math operations #96

Closed
jmirabel opened this issue Mar 16, 2016 · 23 comments
Closed

Using the Eigen library for math operations #96

jmirabel opened this issue Mar 16, 2016 · 23 comments
Milestone

Comments

@jmirabel
Copy link

Hello,

I have experimented integrating the Eigen library and I'm wondering whether you are interested in the resulting code.
Below are more details.

Motivation

The greatest advantage in using Eigen in FCL is that it avoids creating non-necessary temporary matrices (and vectors). The following code:

Vec3f a, b;
Vec3f c = a + b;

is, with FCL vectors, compiled as:

tmp <- a.operator+(b);
c <- tmp

whereas, with Eigen vectors:

c <- a.operator+(b)

This becomes more interesting on more complex expressions. See this for more details on temporary elimination and lazy assignment.

Preliminary results

So far, I have only integrated Eigen in Vec3f and Matrix3f and the improvements are mostly marginal, from 1% to 10% on the basic tests I did. The advantages of the method are:

  • No copy required for users needing Eigen in a library which depends on FCL.
  • A greater gain can be expected by relying on Eigen for other math classes such as Quaternion3f... I did not do any profiling to check what could be done.

Implementation details

There are very few impacts in the current code. Three parts of the code needs to be changed, whenever the prepocessor variable FCL_HAVE_EIGEN is 1:

  • Classes Vec3f and Matrix3f (so far) are declared based on Eigen, with the exact same interface. (the main problem is the inplace transpose in FCL)
  • Class TVector3 needs one additional operator*
  • Conditional operator ?:; does not work with expressions of matrices. For instance,
    b = (condition)?a:-a; must be changed in if (condition) b = a; else b = -a;
@jmirabel
Copy link
Author

It seems there is some interest in this. I'll wait to see the conclusion of the discussion #97. My experiments were done on top the humanoid-path-planner/hpp-fcl fork, mentioned in #97.
Thanks

@traversaro
Copy link
Contributor

I have a question related to this as a (future) user of FCL in a project that is also using Eigen.

As far as I understood (from #111 (comment)) the idea is to use Eigen classes in the FCL public headers. However FCL is distributed in Debian and Ubuntu official repositories (https://packages.debian.org/source/sid/fcl), so it would be expected for stable releases of FCL to define an ABI and be backward compatible with it (for every ABI breakage, a new debian package would need to be introduced).

However, Eigen is an header only library and the ABI of its classes depend on some preprocessor options (for example regarding alignment) that FCL should define, either at the C++ source code level or at the build system level. However if another library uses both Eigen and FCL , and defines a preprocessor symbol that is changing Eigen ABI, then the FCL ABI could be changed depending on where the preprocessor symbol is defined. In a nutshell how it is possible to define an ABI for a public header that is using Eigen classes?

@jmirabel
Copy link
Author

jmirabel commented Apr 4, 2016

A quick answer on Eigen ABI backward compatibility:
If you do not specify certain macros " with major effects", Eigen is ABI and API backward compatible [1]. Some details of what shouldn't be done can be found in [2].

The question of how this is done is a bit technical and I do not know the full details. Very roughly, the algorithm for all operations (sum, difference...) are not in the classes themselves, but in struct. There are structs for every combination of flags (with/without SSE / OpenMP...). When you add two matrices A and B created by the FCL binary (so the FCL ABI), the product will be compiled in the generated binary, with possibly different options (so using a different struct) but it will be compatible (it merely takes as input a pointer and at most two integers).

[1] http://stackoverflow.com/questions/35400033/eigen-3-backwards-compatibility
[2] http://eigen.tuxfamily.org/dox/TopicPreprocessorDirectives.html

@traversaro
Copy link
Contributor

I found the statement on ABI compatibility in [2] quite confusing. In particular, EIGEN_DONT_ALIGN_STATICALLY is not listed under the macros "with major effects", but I guess it changes the ABI, as stated in Eigen documention in [1] . We should anyway consider both the ABI of the Eigen functions (that can contain the strategy that you described) and the ABI of FCL classes that contain Eigen classes (that instead is directly related to the memory layout of Eigen classes).

In general ABI issues are quite tricky, and for my limited experience I am quite skeptical of ABI-compatibility claims that are not backed by a ABI compliance checker actively running on all the possible configurations of the project. Anyway I really hope that my fears are not well founded!

[1] http://eigen.tuxfamily.org/dox/group__TopicUnalignedArrayAssert.html
[2] http://eigen.tuxfamily.org/dox/TopicPreprocessorDirectives.html

@sherm1
Copy link
Member

sherm1 commented Apr 4, 2016

Any thoughts on this, @j-rivero?

@j-rivero
Copy link
Contributor

j-rivero commented Apr 5, 2016

Any thoughts on this, @j-rivero?

Speaking about the use of Eigen or any other math library out there, I'm in favor of using common libraries instead of implementing own code for doing the same.

Changing FCL to use Eigen3 would be a major change for the library, so yes, the API and ABI will probably change. This is expected in the life cycle of a libraries, our work of packaging in debian is to provide users a good transition for this changes.

Please note that when you read about Eigen ABI/API compatibility most of the documents could refer to the major bump from Eigen2 to Eigen3, which is ongoing since some years ago and has been a major change for users.

I don't see problems with API stability if fcl changes to Eigen3. The plan is to stay API compatible during the whole Eigen3 series.

In general ABI issues are quite tricky, and for my limited experience I am quite skeptical of ABI-compatibility claims that are not backed by a ABI compliance checker actively running on all the possible configurations of the project. Anyway I really hope that my fears are not well founded!

Silvio, your fears are (now and generally) well founded: ABI problems are quite problematic to detect. I've expressed my thoughts about the need to implement an ABI checker for the CI pipeline.

@traversaro
Copy link
Contributor

Just to clarify : I am not concerned of the API/ABI changes due to switching to Eigen3 or between different minor versions of Eigen3, those are part of the normal software cycle.

I was instead concerned of the incompatibilities between FCL compiled as it is, and a third library that is using Eigen with a ABI-changing configuration option, such as EIGEN_DONT_ALIGN_STATICALLY, that would change the ABI of FCL as seen by the third library, even if FCL ABI is not changed at all. This would result in mysterious run-time crashes, even if both FCL and the third library were compiled at the same moment on the same machine!

However this configuration is quite esoteric, so I hope that just warning the users of FCL against using ABI-changing Eigen3 definitions will be enough.

@j-rivero
Copy link
Contributor

j-rivero commented Apr 5, 2016

I was instead concerned of the incompatibilities between FCL compiled as it is, and a third library that is using Eigen with a ABI-changing configuration option, such as EIGEN_DONT_ALIGN_STATICALLY, that would change the ABI of FCL as seen by the third library, even if FCL ABI is not changed at all. This would result in mysterious run-time crashes, even if both FCL and the third library were compiled at the same moment on the same machine!

I understand your point and, without looking more into details, I think that it could happen depending on how types are exposed in the public API and how variables are shared between different libraries. I did not heard of any problem like that at this moment nor can find any bug about this. Given the large tradition on the Eigen project and its popularity it would be weird if we are the first that find it.

@traversaro
Copy link
Contributor

@j-rivero I think you are right. To wrap up: if all the libraries packaged in Debian/Ubuntu uses Eigen without the preprocessor options that change the ABI of Eigen classes (in a sense, if they use the "default" ABI of Eigen) everything should be ok. If instead someone uses the "non-standard" ABI of Eigen, they should be ready to face the consequences in terms of interoperability with other libraries. : )

@traversaro
Copy link
Contributor

I recently found this bug in Eigen Bugzilla that is quite relevant to the discussion that we had : http://eigen.tuxfamily.org/bz/show_bug.cgi?id=422 .

TL; DR:

Some options do break ABI compatibility. EIGEN_DEFAULT_TO_ROW_MAJOR is one of them, but not the only one. Another is EIGEN_DONT_ALIGN_STATICALLY, for example. I think that this is a documentation/communication issue. These features are very useful to some people who know what they're doing but clearly they should not be used in code that needs to link with arbitrary other Eigen-using code.

@sherm1
Copy link
Member

sherm1 commented May 24, 2016

Interesting -- thanks! I'd say that makes it clear we should take all the Eigen defaults when we switch FCL to use it.

@jslee02
Copy link
Member

jslee02 commented Jul 23, 2016

Any update on switching to Eigen? I'm willing to help if needed.

@sherm1
Copy link
Member

sherm1 commented Jul 23, 2016

I am hoping to have some people at TRI able to work on this in the next month or so -- do you want to get started earlier? One of the goals I am hoping to achieve is to end up with code that is templatized by the Eigen scalar type so that we can use AutoDiffScalar to get derivatives. That's important for use by Drake and would probably be generally useful. Since Eigen is fully templatized that should come for free with the switch (but could easily be defeated by assuming double here and there).

@jslee02
Copy link
Member

jslee02 commented Jul 24, 2016

I could start like within a couple of weeks.

I must admit I don't understand the exact meaning of templatizing by Eigen scalar type. This is my best guess (let's ignore the naming):

#ifdef FCL_DOULBE_PRECISION
  using real = double;
#else
  using real = float;
#endif

using Vector3 = Eigen::Matrix<real, 3, 1>;
// and so on

Please let me know if this is different from your expectation.

@sherm1
Copy link
Member

sherm1 commented Jul 24, 2016

I confess I haven't thought this through very well for FCL. The goal would be to be able to calculate derivatives like partial(distance)/partial(pose). To do this with autodiff requires being able to maintain several instantiations of FCL objects templatized with different scalar types, in the same compilation unit. So the #ifdef FCL_DOUBLE_PRECISION method wouldn't work. In Drake it requires templatizing all the high-level classes, for example template <typename T> class RigidBodySystem. Then we instantiation with T=double or T=Eigen::AutoDiffScalar<...>. Generally most of the computation occurs with the double instantiation, then at a point where derivatives are needed we switch to the other instantiation, intialize it with the same configuration as the double one, then execute to get derivatives.

Very likely this could be done in several passes -- the first could simply be replacing all computations with Eigen, templatized by a compile-time scalar type as you proposed above, then a second pass to make the real type a template parameter and make it get passed through in place of the compile-time scalar.

@jslee02
Copy link
Member

jslee02 commented Jul 24, 2016

Thanks for the clarification! I have a better understanding, and the derivatives feature sounds useful to me as well.

Let me implement the first pass and see how the second pass would go.

@jmirabel
Copy link
Author

I worked on this topic some time ago. I started modifying the FCL math classes so that the API matches the one of Eigen. I can give you what I did as a starting point.

@jmirabel
Copy link
Author

jmirabel commented Jul 25, 2016

About the automatic differentiation, how does Eigen::AutoDiff handle non-linear spaces ?

FCL uses quaternions so a pose is 4 parameters, but the derivative is only 3 DOFs. And it is not as straightforward to get the derivative on the special orthogonal group as it is on a vector space...

@sherm1
Copy link
Member

sherm1 commented Jul 25, 2016

@jmirabel: autodiffing quaternions does not present any special problem. You may be thinking about representing a quaternion time derivative (4 numbers) as an angular velocity (3 numbers). Autodiffing gives partial derivatives, not total derivative. It is a purely mechanical process that is essentially repeated application of the chain rule and works for any computation.

@jmirabel
Copy link
Author

Yes, I meant 4 parameters and 3 DOFs, sorry. I updated my comment.

@sherm1
Copy link
Member

sherm1 commented Jul 25, 2016

Yes, I meant 4 parameters and 3 DOFs, sorry.

Actually 7 was right for "pose" since that is usually defined to include both translation and rotation, and 6 for translational and rotational velocity. In my response I just mentioned the rotational part because that's where the special orthogonal group you mentioned exists. Anyway, did I answer your actual question?

@jslee02
Copy link
Member

jslee02 commented Sep 9, 2016

FCL is now based on Eigen since #150.

@jslee02 jslee02 closed this as completed Sep 9, 2016
@jmirabel
Copy link
Author

jmirabel commented Sep 9, 2016

This sounds really good. Thanks for this job !

Le 9 sept. 2016 06:27, "Jeongseok (JS) Lee" notifications@github.com a
écrit :

FCL is now based on Eigen since #150
#150.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#96 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACRw8VI42CtdUP0VAQh_lk6oCXHt-R3Tks5qoOA3gaJpZM4Hx-lD
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants