Non-const getter for coeffs()

[markusgft]

Dear all,
I am working on an optimal control library which supports both native Eigen-types and manif-types, as well as optimal control on manifolds.
I am trying to make things as generic as possible, i.e. write my code such that Eigen-types and manif-types are interchangeable where possible.
One of the unfortunate obstacles in the way is that manif does not have a non-const getter for coeffs().
Do you see any major obstacles for adding such a feature?
Any thoughts and discussion on this would be highly appreciated.
Best!

[artivis]

Dear @markusgft,

Indeed Lie group objects do not expose a public non-const coeffs(); note however that tangent objects do. This API was one of the earliest design choice.

The rational is quite simple, we try to prevent the user from assigning values that do not respect the group constraint(s) (e.g. non-normalized quaternions). By returning a non-const reference to the coeffs vector, we do not have the possibility to check the validity of the values at assignment - as we do e.g. in the constructor. We could check it further down the line in any/all operation function but then the issue would be difficult to track.
Note that there is actually already a workaround to this rule with a public function Scalar* data() which was needed to interface with some solvers. But if it was me, I would make it protected too...

Right now I can't think of a good path to make non-const coeffs() public given the aforementioned constraint, but I'm definitely open to discuss the matter and/or workaround (maybe a void set_coeffs(...)?).

@joansola do you have some input on this matter?

[markusgft]

Yes, I see that this design decision makes sense from a point of view, where you want to avoid that the user does anything "uninformed". However, I believe that only people who know what they are doing and are familiar with the basic math of manifolds are using this library.
Of course I am aware that my request is a bit special -- in particular, all I would wish for is as much overlap as possible with the Eigen API, in order to avoid unnecessary specializations our own library, the control-toolbox. Let me give you more context: I am currently working on the next major release of the control-toolbox, which will come with full manifold support for optimal control, trajectory-optimization and state estimation. I my development version of manif, I had to introduce a number of extra features to the API, which I probably should discuss with you guys for potential back-porting any time soon. Non-const access is one of them...
It may also be worth considering, btw, that sometimes we would like to avoid the numerical checks for consistency entirely, for example when invoking our auto-diff tools. By the way, Eigen has also enabled users to get non-const access to the coeffs of the quaternion class, see e.g. here, so it could be considered good pratice ;)

[joansola]

Hello all.

I see the point raised by Markus (thanks Markus) and I think it makes sense. Very often automatic checks and prohibitions that seek safety against misuses are compromised by other opposing needs such as portability, compatibility, speed, and other.

Since Manif is intended as a tool library, it makes sense also to assume that its users know what they are doing, so I'd say it is OK to leave the user satisfy the unit constraint on quaternions.

Finally, if we want to have different levels of protection for different users, the public/private character of parts of the API could be controlled at compile time with some flag on the CMAKE file. I'm no pro programmer and I am sure there might be more elegant ways to do so, but this one is simple.

[andre-nguyen]

@joansola

Finally, if we want to have different levels of protection for different users, the public/private character of parts of the API could be controlled at compile time with some flag on the CMAKE file. I'm no pro programmer and I am sure there might be more elegant ways to do so, but this one is simple.

In some_class.h

class SomeClass {
  public:
  ...
    #ifdef MANIF_SOME_CLASS_PLUGIN
    #include MANIF_SOME_CLASS_PLUGIN
    #endif   
  private:
  ...
}

And then @markusgft would write in some_class_extension.h

Type* someNonConstTypeAccessor() { return &the_thing_; }

And then to use SomeClass you would need to do

#define MANIF_SOME_CLASS_PLUGIN "some_class_extension.h`
#include "manif/some_class.h"

[joansola]

@andre-nguyen I see your point, although not in the details.

I guess that the #define in SomeClass is a typo and should be a #include, no?

Or is it that the #ifdef should be a #isndef?

[andre-nguyen]

Yes! Sorry, edited that. Basically its the same extension system Eigen has

http://eigen.tuxfamily.org/dox-3.2/TopicCustomizingEigen.html

[joansola]

Cool! Thanks for this input!

[artivis]

Hi all,
Allow me to answer between the lines.
@markusgft

where you want to avoid that the user does anything "uninformed". However, I believe that only people who know what they are doing and are familiar with the basic math of manifolds are using this library.

Well sometimes even "informed" users make tiny mistakes that takes way too much time to figure out - coughing 😄. Although I could agree with your point.

all I would wish for is as much overlap as possible with the Eigen API

Having an Eigen-like API is also a design objective of manif. I guess that's another argument in favor of a non const coeffs...

Let me give you more context: [...] control-toolbox

I'm really excited to hear about that and I'm looking forward to the manif integration and to trying it myself.

I had to introduce a number of extra features to the API, which I probably should discuss with you guys for potential back-porting any time soon.

We'd be happy to know which extensions and how we could integrate them upstream. Let us know!

sometimes we would like to avoid the numerical checks for consistency entirely

I totally agree with you. Note that MANIF_CHECK is a macro for this very reason... This is on my todo list and it is fairly easy to implement so I'll look at it asap.

Eigen has also enabled users to get non-const access to the coeffs of the quaternion class, see e.g. here, so it could be considered good pratice ;)

I don't know if I would personally consider that good practice :D. But we do publicly expose normalize therefore we could consider non const coeffs its counter-part.

@joansola

so I'd say it is OK to leave the user satisfy the unit constraint on quaternions

Ok then it looks like we agree on making this API change. However,

if we want to have different levels of protection for different users, the public/private character of parts of the API could be controlled at compile time with some flag on the CMAKE file.

I wouldn't do that. Changing the API based on a flag is prone to confusion more than anything. I'd rather make the function public and write a good old warning in the doc.

@andre-nguyen
Thanks for the MANIF_SOME_CLASS_PLUGIN suggestion (ala Eigen) but here it feels like killing a fly with a rocket. And I'm not sure this scheme is really necessary/desirable for now. Maybe in the future if it proves useful.

So, to conclude, I've been convinced to make an API change and have a public non const coeffs. This will actually simplify some bits. Adding it to the todo list 👍.

Finally @markusgft, as you seem to be a manif power user, I'd love to hear your opinion on #116 and more especially on the design change I propose here concerning the Jacobians. Note that I'd likely have the new Jacobian type inherit from Eigen to have a fully qualified type rather than being a 'simple' traits.

[artivis]

@markusgft sorry I forgot to ask you, which 'auto-diff tools' are you using? It would be great to confirm that manif supports other tools than Ceres::Jet on the readme page. I meant to integrate CppAD for a long time but never got to finish that.
Side question, do you think it would it be useful to your control library or other to have auto-diffed second derivatives?

[markusgft]

Dear all,
first of all, thanks a lot for the quick and helpful responses, and I am really happy to hear that you guys are so open-minded towards helping us out!

• Autodiff: The control-toolbox currently uses CppAD and CppADCodeGen as main tools for algorithmic differentiation and code generation. Some users also employ CasADi . I must admit that I have not yet tested manif for AD-compatibility thorougly, in my current implementation examples I use analytical derivatives. However, AD-compatbility is a very important point on the agenda.
  @artivis auto-diffed second derivatives are indeed required for many optimal control applications. All algorithms in the control-toolbox require second-order gradients of the cost functions, we currently have another development thread which targets control algorithms that also use second-order derivatives of dynamics and inequality constraints. So therefore, I would classify second-order derivative support as "important to both the community and my own project".

• Left vs right Jacobians, Implement left-Jacobians #116. Thanks for bringing this up, this is indeed an important discussion. Personally, I have set the development of the control-toolbox set up such that it exclusively uses local perturbations, right plus and minus and right Jacobians. Our main argument why we go down that path is because we are using a family of solvers based on (local) sequential linear-quadratic programming, see e.g. here or here , which are expressed in a differential formulation in their most natural and wide-spread form. Personally, I have made a number of experiments where I compared "global" and "local" formulations. In practice, the "local" formulation does lead to a few advantages, which is why we have almost exculsively adopted that variant.

But what about addressing all those issues in separate threads? I will start opening separate feature requests for the additional small changes that I would need, and then I can continue discussing there!
As for the non-const() getters, I am really looking forward to use them!