Pinocchio: updating only the momentum quantities.

[proyan]

We need a function in crocoddyl, that updates the centroidal momentum hg, without additional computations Currently the version that we have (computeCentroidalDynamics) computes the kinematics as well, and thus is not as efficient.

I'm willing to implement it here, but I wish to affirm that there is an appetite for such a function in Pinocchio. What do you think?

[jcarpent]

This is fine for me. I can also handle it before doing a new release. What do you think?

[cmastalli]

There is two ways of doing it, I'm not sure what it's the Pinocchio style:

1. updateCentroidalDynamics(model, data)
2. computeCentroidalDynamics(model, data) #we don't pass q and qd

Which option should we pursue @jcarpent ?

[jcarpent]

I think the best naming is updateCentroidalDynamics as done in other places in Pinocchio.

[proyan]

If you can handle it, I'm not going to say no :smiley:

[gabrielebndn]

My bit: I think computeCentroidalDynamics(model,data). Most methods (think of forwardDynamics) can work with or without the configuration.
This way we could establish a nice pattern in all methods: if the configuration is passed, then it is used to compute the kinematics, otherwise the kinematics is not computed

[jcarpent]

@gabrielebndn Not for forwardDynamics, because you don't know if you want the zero, first or second kinematics. The same applied for computeCentroidalDynamics. One has to understand the update by update from the current value in Data.

[cmastalli]

If you can handle it, I'm not going to say no smiley

Unfortunately, I don't have time. Thanks for making this effort @proyan :smiley:

[jcarpent]

@cmastalli Rohan was talking to me ;)

[proyan]

@jcarpent Worth the try :D
No problem, I'll make the PR within next week. I'll prefer updateCentroidalDynamics.
By the way, I don't think that I can do it with simply updateCentroidalDynamics(model, data). Correct me if I am wrong, we'll need v in order to update the centroidal dynamics. And just inputting v, without q seems bad.

[gabrielebndn]

For forwardDynamics and the like, we assume that the forward kinematics has been computed up to the first time derivative. So I do not think you need to take v into account. As long as the data.v[i] are filled with the correct values, I do not think you need anything else

[jcarpent]

The answer from @gabrielebndn is correct. But, I was thinking to add a enum to say that we want:
computeCentroidalDynamics(model,data,q,v) maching computeCentroidalDynamics(model,data,FIRST_ORDER) and computeCentroidalDynamics(model,data,q,v,a) matching computeCentroidalDynamics(model,data,SECOND_ORDER).

What do you think?

[gabrielebndn]

@jcarpent ooh, now I understand the issue... You have two computeCentroidalDynamics, one being first order and the other one second order, but without the arguments you cannot tell which one you want...
In my opinion, I've always found the name computeCentroidalDynamics to be confusing... You could rename them computeCentroidalMomentum and computeCentroidalMomentumTimeDerivative, it would be more informative and there would be no ambiguity when no arguments are provided...

[jcarpent]

Sounds a good idea indeed. I will provide such functions.

[proyan]

I agree with @gabrielebndn, centroidalDynamics is quite vague.
So I create the new function with computeCentroidalMomentum(model, data), and deprecate and rename the original functions?

[jcarpent]

As I mentioned above, I will take in charge @proyan. I have to apply some other modifications elsewhere.

[jcarpent]

@proyan But, if it is urgent for you, please go ahead, I will adjust my code accordingly.

[jcarpent]

Anyway, I think you can keep the current functions, but the two signatures you need:

computeCentroidalMomentum(model,data)
computeCentroidalMomentumTimeDerivatives(model,data)

because

computeCentroidalDynamics(model,data,q,v)
computeCentroidalDynamics(model,data,q,v,a)

are clear enough I think, no?

We may add shortcut with the naming proposed by @gabrielebndn.
@proyan @gabrielebndn @cmastalli What do you think?

[jcarpent]

After thinking a bit, it will too much redundant to cope these namings. So, it would nice indeed to mark the former naming as deprecated.
Sorry for the noise ;)

[gabrielebndn]

I think computeCentroidalDynamics is a bit confusing. I suggest imoplementing the four signatures

computeCentroidalMomentum(model,data)
computeCentroidalMomentumTimeDerivative(model,data)
computeCentroidalMomentum(model,data,q,v)
computeCentroidalMomentumTimeDerivative(model,data,q,v,a)

and deprecating

computeCentroidalDynamics(model,data,q,v)
computeCentroidalDynamics(model,data,q,v,a)

which will just call computeCentroidalMomentum and computeCentroidalMomentumTimeDerivative, respectively. It is better if the methods keep the same names in the version with and without arguments

[jcarpent]

Solved by #944.