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

Introduce Eigen-style transpose() for Tangent-types #142

Open
markusgft opened this issue Jun 17, 2020 · 26 comments
Open

Introduce Eigen-style transpose() for Tangent-types #142

markusgft opened this issue Jun 17, 2020 · 26 comments
Assignees
Labels
API API change enhancement New feature or request

Comments

@markusgft
Copy link

Dear all,
what do you think of introducing an Eigen-style transpose() call for manif tangent types?
The context of the question is the same as in #137.
So far, I helped myself with introducing the following public method to my personal clone of manifs tangent_base.h:

auto transpose() const {return coeffs().transpose();} 

Note that it in fact returns the transpose of the underlying data type, i.e. the transpose of an Eigen-matrix.
I would like to hear your opinion regarding such a feature, and like to suggest that this could also be helpful for other users?
Best!

@joansola
Copy link
Collaborator

Just for clarification. I suppose you are aware that all our tangent types are Cartesian vectors, that is Matrix<T, size, 1>, and that we do not expose normally the true Lie algebra types. Does this affect your comment / request in some way?

@markusgft
Copy link
Author

markusgft commented Jun 17, 2020

No, I don't think that affects the principal idea. Maybe an application example: what I would like to achieve is that one can evaluate quadratic forms like the following ...

manif::SE3Tangentd t = /* ... some init. */; 
Eigen::Matrix M (6,6);
M = /* ... some init. */;

// We should be able to do this..:
Eigen::MatrixXd res = t.transpose() * M * t;

... while using pure Eigen-syntax! Does that clarify the idea?

@joansola
Copy link
Collaborator

Yes it does! Thanks. I'm mostly concerned with the math of the library. Jeremie is the one involved in the design / development of the library. Let us see what he says.

@artivis
Copy link
Owner

artivis commented Jun 17, 2020

Hi all,
looks like this request, just like #143, if about bringing some of Eigen's vector API to the tangent classes. I'm wondering if it would make sense for the TangentBase class to 'simply' directly inherit from Eigen::Matrix<T, size, 1>. This would bring a lot to the API space but since the tangent types are Cartesian vectors maybe it make sense. We would also benefit from some Eigen optimizations here and there...
@joansola thoughts?

@artivis artivis added API API change enhancement New feature or request labels Jun 17, 2020
@artivis artivis self-assigned this Jun 17, 2020
@joansola
Copy link
Collaborator

joansola commented Jun 17, 2020

Yes, it makes perfect sense and you save a lot of work.

I think the base API from Eigen can be exposed partially also, should some parts of it become irrelevant or dangerous for any reason, using the using and delete keywords or similar, but you know better than me about these things.

@markusgft
Copy link
Author

Inheriting from Eigen::Matrix seems like a good idea, I did not have that on my radar, but it probably makes sense for the Tangent class!

@markusgft
Copy link
Author

Dear @artivis @joansola
I wanted to use the opportunity to follow up and further energize the idea of deriving the Tangent classes from Eigen::Matrix. I think that would be a fantastic feature, which would help a lot of people to run advanced linear algebra routines in the tangent space with ease.
If you are considering implementing it, it would be very helpful if you quickly comment on potential timelines, as to simplify our own planning of feature development for the control toolbox.
Best, and thanks a lot in advance!

@artivis
Copy link
Owner

artivis commented Jun 25, 2020

Hi @markusgft,
I am quite sold to the idea of the Tangent class inheriting from Matrix (MatrixBase most likely) however I anticipate this not to be simple and fairly time consuming. So that you know, I develop manif on my free time, therefore it is not always easy to find large blocks of hours to properly focus.
I will have a look at this over the weekend to get a better idea of the implications and hopefully provide you with an estimated time of landing.
Cheers.

@markusgft
Copy link
Author

No worries, I fully understand (it's the same for me and the control toolbox) and I certainly did not want to appear too demanding!
Let me know if there's anything I can contribute to the effort.
Cheers.

@joansola
Copy link
Collaborator

From my side there's not much I can do. But I'd like to see this evolving since I believe this would be a powerful upgrade of the library.

@artivis
Copy link
Owner

artivis commented Jun 29, 2020

Hi guyz,
a quick update to let you know that I looked at the inheritance thingy over the weekend and as I expected it won't be simple. Actually I don't even know how to deal with it atm. The problem is the following: manif design, much like Eigen's, separates the interface (Base classes) from the actual memory allocation (say e.g. final SE2 class). This scheme allows for instance to easily specialize Eigen::Map for manif object through inheritance (a Map<SE2> inherits from SE2Base). If TangentBase (or any intermediate base for that matter) was to inherit directly from eigen::Matrix this separation of concern would be broken (a Map<SE2> object would allocate to underlying vector even if it doesn't use it...). I gave a quick shot at inheriting from MatrixBase but that raises all kind of issues on Eigen's side (traits that are not specialized for manif objects etc). I'm not sure how to get around this, maybe any of you know of a Eigen super expert? I'll also ask for guidance on Eigen's forum asap.
Depending on how urgent is the API extension, we may implement it directly in manif for now while resolving the inheritance difficulty.

Thoughts?

@joansola
Copy link
Collaborator

joansola commented Jun 29, 2020

Here's my thought.

Implementing most of the Eigen API on top of manif is maybe a dumb option but very effective. It might be tedious but other than that it's a fine way to go.

I wonder if we can design a macro of the style:

MANIF_ADD_EIGEN_METHOD(MethodName, __VA_ARGS__)

that would act as a bridge to call the Eigen method.
So that now we just need to add method after method.

I'd say also to put all this in a separate file to be able to cut it out one day.

@markusgft
Copy link
Author

markusgft commented Jun 29, 2020

Hi @artivis, why should you inherit from MatrixBase rather than from Eigen::Matrix? Do you think exploiting static polymorphism is going to make such a massive difference here? Inheriting from Eigen::Matrix directly might be slightly less efficient, at the same time we do it in the control-toolbox, too, and do not have bad experience with that design.

@artivis
Copy link
Owner

artivis commented Jun 29, 2020

@joansola :
yeah re-implementing Eigen's API is not the way to go in the long run but if we only need a couple functions quickly it is not an issue.
@markusgft:
With the current design, if TangentBase inherits directly from Matrix then every Map<ManifObject> will have a useless memory block as a member. If you look at e.g. Map<SE2> specialization, you will notice that while specializing for SE2 it actually inherits from SE2Base. As a result it has both TangentBase and SE2Base APIs but no actual Eigen::Vector member (its data_ is actually a Eigen::Map, no memory allocated). I'm not looking at exploiting Eigen's static polymorphism as much as trying not to redesign entirely manif and keep benefiting from its current design (largely inspired by Eigen of course).
Now there are other approaches I'll have to investigate but it won't be a 'quick fix'.

@markusgft May I ask you to list the actual functions you are missing from Eigen? Is it only transpose and operator()(int) at this point? If so I can add them in the meantime.

@markusgft
Copy link
Author

Dear @artivis, sorry for introducing such a long delay before my response, now I finally manged to come back to this thread. Here is a (preliminary) list of things that I currently have "hacked" myself into manif, and which would be fantastic to have in the main library.

  1. transpose() operator for tangent types (see top of this thread)
  2. Operator for accessing/writing single elements of the tangent types, e.g. Scalar operator()(int idx) {return coeffs()(idx);}
  3. .noalias() and .eval() methods, see neighbouring thread (low priority).
  4. operator+ for manifold types, e.g. a+b to be used as a replacement for a.compose(b). Here, I fully understand why you wanted to make the use-case explicit by calling it "compose", however, providing this operator is required for using boost-odeint on manif types. (Note: this is just a nice to have, I can also overload it in my main library).
  5. operator* for both orders of multiplication between a tangent-type t and a scalar s. I noted that as of now, only one direction of multiplication is supported, namely t*s. For compatibility with boost-odeint integration, we also need to have s*t.
  6. I also had to introduce the following workaround in tangent_base.h:
// this is required to allow for t.transpose() * M * t using Eigen API and manif tangent types:
/** // for  example:
    manif::SE2Tangentd t_test;
    t_test.setRandom();
    Eigen::Matrix<double, 3, 3> M_test;
    M_test.setRandom();
    std::cout << t_test.transpose() * t_test << std::endl;
    std::cout << t_test.transpose() * M_test * t_test << std::endl;
 */
template <typename _Derived, typename _EigenDerived>
auto operator*(const Eigen::MatrixBase<_EigenDerived> &m, const TangentBase<_Derived> &t) {
  return m * t.coeffs(); 
}
  1. Lastly, I think I had to include this operator (or similar, but I would need to verify that it is still required), to allow writing back to Eigen types...
  template <typename _EigenDerived>
  Eigen::MatrixBase<_EigenDerived>& operator =(const TangentBase<_Derived>& t) const;

Happy to discuss if you have questions! All the best!

@artivis
Copy link
Owner

artivis commented Jul 2, 2020

Hi @markusgft,
just a quick reaction to your fourth point,

operator+ for manifold types, e.g. a+b to be used as a replacement for a.compose(b).

For manifold and tangent types, the operator + is overloaded on-top of rplus/lplus functions. Introducing the overload you are proposing would be, at least, confusing. Note that the operator * is the current compose overload (a.compose(b) -> a*b).

The rest of the comment seems reasonable. I'll come back to it more in detail later on.

@artivis
Copy link
Owner

artivis commented Jul 3, 2020

Hi all,

for completeness, the following design seems to work:

struct TangentBase; // unchanged
struct SE2TangentBase : TangentBase; // unchanged
struct SE2Tangent : SE2TangentBase<SE2Tangent>, Eigen::Matrix<Scalar, 3, 1>; // change

pro: we get to inherit the Eigen API at the most user-facing level
cons:

  • Eigen API not accessible at neither SE2TangentBase nor TangentBase level limiting the possibility to write generic code. E.g. not doable:
    template <typename T>
    auto mtm(const TangentBase<T>& t) { return t.transpose() * t; }
  • Each and every final manifold class (here SE2Tangent) inherits from Eigen::Matrix independently

The first cons is imho the most annoying one.
The last cons can be mitigated in terms of code verbosity with a proxy class, e.g. something along the line

template <template <typename Derived> class Base>
struct proxy : Base<Derived>, Eigen::Matrix<Derived::Scalar, Derived::Dim, 1>;

struct SE2Tangent : proxy<SE2TangentBase<SE2>>;

@joansola
Copy link
Collaborator

joansola commented Jul 3, 2020

  1. I suppose you mean Matrix<Scalar, 3, 1>
  2. Generic code is nice but maaaybe it does not happen so often. Of course your example would like to expand the API of TangentBase with a method that does not exist in Eigen -- otherwise, you don't have generic code, but you have the same API for all classes, so it's pretty much the same user-wise.
  3. I do not see anything intelligible in the template < template < typename .... bit of code ! :-(

@artivis
Copy link
Owner

artivis commented Jul 3, 2020

@joansola

  1. Oups, I meant Matrix<Scalar, 3, 1>. Fixed it.
  2. I coded the TESC-planner using generic code so that moving from a mobile-base (SE2) to an arm end-effector (SE3) was nearly effortless. I think that makes the case for generic code. And the same goes for the couple algorithms in manif. I'd argue that generic code is under-utilized at the moment rather than not so useful. So my feeling is that any 'higher-lever' functions (such as transpose) should be available at the TangentBase level too, as it is the case now. But of course that also makes this issue more difficult.
  3. Nevermind this gibberish, the point being that it allows to write struct SE2 : proxy<SE2Base<SE2>> instead of struct SE2 : SE2Base<SE2>, Eigen::Matrix<Scalar, 4, 1>.

@joansola
Copy link
Collaborator

joansola commented Jul 3, 2020

  1. Again: did you mean Matrix<Scalar, 3, 1>? The tangent space se2 has 3DoF! The real/imag thing happens in the group, not in the tangent space! Or so it should

@artivis
Copy link
Owner

artivis commented Jul 3, 2020

@joansola You are right, I was somehow blocked on SE2 rather than SE2Tangent. Fixing the above examples/comments.

@joansola
Copy link
Collaborator

joansola commented Jul 3, 2020

  1. OK for your view of the generic code.
  2. Yep I see.

@markusgft
Copy link
Author

Hi @markusgft,
just a quick reaction to your fourth point,

operator+ for manifold types, e.g. a+b to be used as a replacement for a.compose(b).

For manifold and tangent types, the operator + is overloaded on-top of rplus/lplus functions. Introducing the overload you are proposing would be, at least, confusing. Note that the operator * is the current compose overload (a.compose(b) -> a*b).

Yes, I can see your point -- I can overload that locally in the CT. I only mentioned that because boost odeint requires the user to overload + in order to work. Another option would of course be to independently commit a set of corresponding operations to boost odeint, but I feel like this would be beyond our reach.

A big +1 here from my side for the generic code argument!

@pettni
Copy link
Contributor

pettni commented Feb 28, 2021

I'm taking the freedom to hijack this issue to pose a related question: why have a manif tangent type at all? To me it seems that defining the tangent type as Eigen::Matrix<Scalar, DoF, 1> would come with some benefits at a fairly small cost:

Pros:

  • Significantly less of code required in the library
  • Get all Eigen methods (such as transpose()) for free
  • Get Eigen lazy evaluation for tangent algebra which, as @markusgft points out, may boost performance

Cons:

  • Certain syntax becomes impossible, for example v + G would have to be written G.lplus(v) and v.exp() would have to be e.g. G::exp(v)

Of course I don't expect you to change such a fundamental part of the library, but I'm curious about the considerations that went into the choice.

@artivis
Copy link
Owner

artivis commented Mar 1, 2021

Allow me to start by being skeptical; I don't quite buy the performance boost argument. I'd need to see actual, significant, numbers.
For instance, the lazy evaluation would only happen for functions that are implemented on the Eigen side since functions that are implemented by manif does not take care of that. I hope to be able to provide some similar mechanisms past #150 but that's truly a lot of work for benefits that seems thin at best.

This being said, in hindsight TangentBase should probably have inherited from Eigen::Matrix or Eigen::MatrixBase, this would have brought all Eigen methods at the tangent scope. It is somewhere on my todo list for manif.

Now, as for the reason of the tangent classes, well, call me old fashion but I like my object-oriented strongly-typed programming language. What's an Eigen::Matrix<double, 3, 1> object? se2? so3? Something else? Who knows.
I guess the main motivations really were the sugar-coated syntax and the enablement of generic programming

template <typename Derived> void foo( TangentBase<derived>& tangent ) { auto g = tangent.exp(Jacobian) ... }

I had a look at Eigen's plugin system at first but I didn't like it much because any function injected is available (somewhat pollutes) the Eigen class for the entire project. All of a sudden your top-level project (of which manif is only a tiny dependency) sees a Eigen::Matrix::exp function; what the hell. Maybe I missed something tho...

Note that Sophus uses plain Eigen vector for it tangent types.

@pettni
Copy link
Contributor

pettni commented Mar 1, 2021

Appreciate the response!

Yeah I'm aware of Sophus and have used it extensively, I overall like manif better but this may be the one thing I prefer with Sophus... :) Also agree that it would be useful to see some numbers for the lazy eval "benefit", my information is based only on reading previous comments in this thread and in #144 .

Regarding generic programming, unless I am mistaken a similar effect could be achieved by templating over the group type. Granted, it's a bit clunky since the compiler can't infer what tangent type tangent is, so the group type would have to be specified as a template parameter when calling the function.

template <typename Group, typename Derived> void foo(Eigen::MatrixBase<Derived> & tangent) {Group::exp(tangent) ... }

I also looked into the plugin mechanism a couple of months ago after reading this thread, but came away with the impression that it wa a bad option. Even inheriting from somewhere in the Eigen hierarchy seemed tricky and I gave up my experiments, but I can't recall the exact reasons for why it was hard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API change enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants