-
Notifications
You must be signed in to change notification settings - Fork 6
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
base: topic/humble-devel/refactor
Are you sure you want to change the base?
Add warmstart implementation for warmstart from reference points #111
Conversation
There was a problem hiding this 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
There was a problem hiding this 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.
Co-authored-by: Arthur Haffemayer <[email protected]>
Co-authored-by: Arthur Haffemayer <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
numpy types Co-authored-by: Krzysztof Wojciechowski <[email protected]>
numpy types Co-authored-by: Krzysztof Wojciechowski <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this 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
Co-authored-by: Krzysztof Wojciechowski <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Krzysztof Wojciechowski <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Krzysztof Wojciechowski <[email protected]>
for more information, see https://pre-commit.ci
There was a problem hiding this 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
The warmstart should generate the initial values for state and control trajectories, | ||
based on the initial robot state and a reference trajectory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
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?
No description provided.