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

Add warmstart implementation for warmstart from reference points #111

Open
wants to merge 32 commits into
base: topic/humble-devel/refactor
Choose a base branch
from

Conversation

kzorina
Copy link
Contributor

@kzorina kzorina commented Dec 11, 2024

No description provided.

Copy link
Collaborator

@ArthurH91 ArthurH91 left a comment

Choose a reason for hiding this comment

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

few comments on order of imports & the test class could have docstrings to explain what's tested in each test, but otherwise looks good

agimus_controller/agimus_controller/warm_start_base.py Outdated Show resolved Hide resolved
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.

There are some issues with the WarmStartBase API.
Take a look at how the MPC is actually using it.
This is the proper API. I am sorry I have not seen it in the previous PR.

agimus_controller/agimus_controller/warm_start_base.py Outdated Show resolved Hide resolved
agimus_controller/tests/test_warm_start.py Outdated Show resolved Hide resolved
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.

When I think of it now, maybe the documentation should be filled in on the WarmStartBase class side and then it can be assumed to be inherited later on.

It should also be extended in the base class and WarmStartReference is still missing a loat of documentation

@kzorina kzorina requested a review from Kotochleb December 13, 2024 12:46
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.

Just a few cosmetic changes. Otherwise, it's ok. I have weird feeling about those asserts, but I will be a hypocrite to tell you to do this and later change my mind

Comment on lines +12 to +13
The warmstart should generate the initial values for state and control trajectories,
based on the initial robot state and a reference trajectory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The warmstart should generate the initial values for state and control trajectories,
based on the initial robot state and a reference trajectory.
The warmstart is expected to generate the initial values for state and control trajectories,
based on the initial robot state and a reference trajectory.

...

def update_previous_solution(
self, previous_solution: list[TrajectoryPoint]
) -> None:
"""Stores internally the previous solution of the OCP"""
"""
This method stores the solution from a previous optimization cycle.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This method stores the solution from a previous optimization cycle.
Store the solution from a previous optimization cycle.

from agimus_controller.trajectory import TrajectoryPoint


class WarmStartReference(WarmStartBase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we add docstring explaining what this warmstart implements?

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