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

OCP Croco Class #112

Conversation

ArthurH91
Copy link
Collaborator

@ArthurH91 ArthurH91 commented Dec 11, 2024

Hello,

Intro

Here's a draft PR on the OCP base class.

After a discussion with @MaximilienNaveau , we agreed that having only crocoddyl might restrict some users. So we decided to have the OCPBase class staying as it is right now, and having a daughter class OCPCrocoBase, which is a simple task to reach a target with the end effector, as a baseline.

In this draft, I'd like to discuss this with y'all. Do you agree with this reasoning?

What's in the PR

  • changes on absolute paths for files relative to mpc
  • modification of the input of solve of the ocpbase (because it takes x_init and x_0, which is already given in x_init)
  • the OCPCrocoBaseClass, as mentionned earlier
  • an OCP Param Base class, that can "generalize" the input of the OCP (ie number of iterations of the solver, qp iters if SQP, error, termination condition ...) after the parsing.
  • a draft on the testing, because i have several issues, especially with the data classes not taking none as input

What I'd like to discuss

  • OCP Param Base class, I believe that's a valid option as output to the parser (be it from ros, yaml or whatever)

Copy link
Contributor

@Kotochleb Kotochleb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, you should avoid introduction of new public function to the abstract class. It was meant to be somewhat fixed so that no one is confused, and we do not end up with compatibility issues like those between for example, default Crocoddyl objects and MiM Solvers.

Additionally your implementation of OCPCrocoBase is not generic at all. It is very specific and you should introduce another class like OCPCrocoReaching where you can impose use of CSQP and your costs. OCPCrocoBase should only implement basic interaction with Crocoddyl and expose maybe new private abstract functions that allow user to choose which solver they want to use and how they want to set up shooting problem object.

Additionally please add docstrigns to OCPBase as you are the first person to extend it, so we will not have to document it later on.

Copy link

@MedericFourmy MedericFourmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern is the function solve: maybe creating croco objects everytime would be too costly.

### Creation of the state and actuation models

# Stat and actuation model
self._state = crocoddyl.StateMultibody(self._rmodel)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I wonder if this is reasonable (in terms of time) to create croco objects (running/terminal costs, Action models etc.) from scratch every time we solve? It's simpler than going through through the croco ocp structure to only but might be a bit slow.
I just checked on a personal codebase a similar function and got:
T = 20 -> 0.7 ms
T = 40 -> 1.5 ms
T = 60 -> 2.7 ms
T = 80 -> 3.8 ms
T = 100 -> 4.5 ms

So it seems that the _state/_actuation creation is negligeable (I'd still put it in __init__ or a setup function) compared to the running cost creation.

@ArthurH91
Copy link
Collaborator Author

My main concern is the function solve: maybe creating croco objects everytime would be too costly.

i totally agree with this, thank you all for the reviews, i'm gonna start working on it tmrw !

Comment on lines 89 to 91
self.running_model_list = self.create_running_model_list()
# Create the terminal model
self.terminal_model = self.create_terminal_model()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are not defined in __init__ with value None and why are they public?

# Create a concrete implementation of OCPBaseCroco
self.ocp = self.TestOCPCroco(self.robot_model_factory, self.ocp_params)
self.ocp.solve(self.state_reg, self.state_warmstart, self.control_warmstart)
# self.save_results()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better. Or even save a pickle with a dict with the numpy objects like so:

results = {
   "states": ' self.ocp.ocp_results.states.tolist(),
   "ricatti_gains": ' self.ocp.ocp_results.ricatti_gains.tolist(),
   "feed_forward_terms": ' self.ocp.ocp_results.feed_forward_terms.tolist(),
}

with open("ressources/simple_ocp_croco_results.pkl", 'wb') as handle:
    pickle.dump(results, handle, protocol=pickle.HIGHEST_PROTOCOL)

Maybe this way if someone loads it, it will be more obvious what is inside of this object

Copy link
Collaborator

@MaximilienNaveau MaximilienNaveau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment on the initial value of the running and terminal model and I am happy.

@MaximilienNaveau
Copy link
Collaborator

@MedericFourmy @Kotochleb @TheoMF Are you happy with the current state?
If yes please approve the PR.

@MaximilienNaveau MaximilienNaveau merged commit 63fd114 into agimus-project:topic/humble-devel/refactor Jan 14, 2025
2 of 4 checks passed
@ArthurH91 ArthurH91 mentioned this pull request Jan 17, 2025
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

Successfully merging this pull request may close these issues.

5 participants