Defining an abstract class for action models and datas - [merged] #480

Closed wxmerkt closed 4 years ago

In GitLab by @cmastalli on Mar 8, 2019, 14:22

Merges topics/action-abstraction -> devel

The main target of this PR is to create an abstraction class for action models and datas as pointed out here #79. The tasks achieved were:

Creation of abstract classes for action models and datas.
Used this class to implement unicycles, LQR and NumDiff classes (both model and data)
Added g and L variables in differential action and action data classes. This is convenient for saving time in some cases.
Used standard naming for AMLQR formulation.
Removed setUpRandom from AMLQR. With this, we preserve the same structure of DAMLQR.
Added recalc option for AMLQR.calcDiff.
Removed UnicycleData classes since we don't need special implementatio (with the abstract it's enough).

In GitLab by @cmastalli on Mar 8, 2019, 14:23

changed title from {-Topics/action abstraction-} to {+Defining an abstract class for action models and datas+}

In GitLab by @cmastalli on Mar 8, 2019, 14:23

changed the description

In GitLab by @nmansard on Mar 8, 2019, 15:27

I think a AM should come with a State. Then nx and ndx can be deduced from the State. nu has to be provided.

def __init(self,State,nu):
  self.State = State
  self.nx = State.nx
  self.ndx = State.ndx
  self.nu = nu

In GitLab by @nmansard on Mar 8, 2019, 15:28

Thanks, nice job. It seems that this MR is merged with !131 , is it not? You should first finalize !131 before proposing this one.

In GitLab by @cmastalli on Mar 8, 2019, 17:11

In GitLab by @cmastalli on Mar 8, 2019, 17:11

added 2 commits

974eb866 - [action] Passing the state as an argument in the action model abstraction
7c8f6ec7 - [action] Random values for full rank matrices

In GitLab by @cmastalli on Mar 8, 2019, 17:12

It's a nice idea. Done!

In GitLab by @cmastalli on Mar 8, 2019, 17:12

resolved all discussions

In GitLab by @cmastalli on Mar 8, 2019, 17:14

I add random generation of orthogonal matrix inside AMLQR. It was a similar issue as reported for DAMLQR (see https://gepgitlab.laas.fr/loco-3d/crocoddyl/merge_requests/131#note_3531).

This PR doesn't contain commit from !131. We can merge it.

In GitLab by @nmansard on Mar 12, 2019, 08:33

Sure? Because you modified some tests using the LQR to rename it. Is it a late patch for a mistake introduced after the initial changes in the AMLQR?

In GitLab by @cmastalli on Mar 12, 2019, 14:21

As discussed orally, there is not connection with !131. You can merge it

In GitLab by @nmansard on Mar 12, 2019, 15:55

merged

In GitLab by @nmansard on Mar 12, 2019, 15:55

mentioned in commit 3269cf78551143253c687ce41039901368bd4766

loco-3d / crocoddyl