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.

Sign up for GitHub

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

Jump to bottom

Topic/mnaveau/buffer #162

Merged
merged 15 commits into from
Jan 30, 2025
Merged

Topic/mnaveau/buffer #162

merged 15 commits into from
Jan 30, 2025

Conversation

MaximilienNaveau
Copy link
Collaborator

In this PR I introduce:

  • the fact that the Points can be compared (usefull in unit-tests and maybe in other places).
  • the fact that the horizon is extracted from a list of dts and not a simple int anymore.
  • the fact that the horizon can be extracted from a specific time inside the buffer.
  • Unit-testing all of this.

What is missing the API change in the mpc.py I guess.

@MaximilienNaveau MaximilienNaveau self-assigned this Jan 28, 2025
@MaximilienNaveau MaximilienNaveau marked this pull request as ready for review January 28, 2025 18:16
jmirabel
jmirabel reviewed Jan 29, 2025
View reviewed changes
Copy link
Contributor

@jmirabel jmirabel left a comment

Choose a reason for hiding this comment

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

Can you make the changes in the MPC class to support the dt factors pls ?
I don't think there is a lot of changes.

agimus_controller/agimus_controller/ocp_param_base.py Show resolved Hide resolved
agimus_controller/agimus_controller/trajectory.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/ocp_param_base.py Outdated Show resolved Hide resolved
jmirabel
jmirabel approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@jmirabel jmirabel left a comment

Choose a reason for hiding this comment

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

Looks ok assuming you fix the opened comments.

Up to you whether to keep the update of MPC class for a subsequent PR.

.gitignore Show resolved Hide resolved
agimus_controller/agimus_controller/trajectory.py Outdated Show resolved Hide resolved
MaximilienNaveau and others added 14 commits January 29, 2025 16:45
@MaximilienNaveau
Start adding the ocp dts
6431555
@MaximilienNaveau
add the horizon_dts as a parameter. Initialize it to be empty.
77008c9
@MaximilienNaveau
Add test for the buffer and ignore the generated paramter file
92e413e
@MaximilienNaveau
Add a clear-past unit-test
99a94d3
@MaximilienNaveau
Add equal operator on the trajectory objects for testing purposes.
8675ec0
@MaximilienNaveau
[Not working yet] Prepare the multi-dts in the buffer
be246af
@MaximilienNaveau
Prepare the buffer for different dts
6ac2f52
@MaximilienNaveau
remove the debug print
d2f497d
@pre-commit-ci @MaximilienNaveau
[pre-commit.ci] auto fixes from pre-commit.com hooks
2789fcb 
for more information, see https://pre-commit.ci
@MaximilienNaveau
Fix the implementation of the buffer to be more simple
a6f3991
@MaximilienNaveau
Change the buffer inheritance to an internal list
f25270a
@pre-commit-ci @MaximilienNaveau
[pre-commit.ci] auto fixes from pre-commit.com hooks
9728624 
for more information, see https://pre-commit.ci
@MaximilienNaveau
Update name of the ocp_param_base class. Fix unit-tests.
4f26986
@MaximilienNaveau
Update the MPC API from the buffer one.
d69eca4
@MaximilienNaveau
Copy link
Collaborator Author

@jmirabel can you re-check again?
If you are ok with that I will merge.

@MaximilienNaveau MaximilienNaveau linked an issue Jan 29, 2025 that may be closed by this pull request
[MPC] create trejectory buffer logic #143
Open
jmirabel
jmirabel approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@jmirabel jmirabel left a comment

Choose a reason for hiding this comment

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

Fine for me.
Not too sure why you renamed the param class which looks specific to crocodyl. But I'm fine with it.

@MaximilienNaveau
Copy link
Collaborator Author

MaximilienNaveau commented Jan 30, 2025 •
edited
Loading

Fine for me. Not too sure why you renamed the param class which looks specific to crocodyl. But I'm fine with it.

I found it very confusing to have a class that do not have the same name as the file name.

Sorry I revert this changes it was too arbitrary.

@MaximilienNaveau MaximilienNaveau merged commit d79adc9 into agimus-project:topic/humble-devel/refactor Jan 30, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheoMF TheoMF TheoMF left review comments

@jmirabel jmirabel jmirabel approved these changes

@kzorina kzorina Awaiting requested review from kzorina

Assignees

@MaximilienNaveau MaximilienNaveau

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MPC] create trejectory buffer logic
3 participants
@MaximilienNaveau @jmirabel @TheoMF