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

Restructure ML component #771

Merged
merged 27 commits into from
Nov 29, 2023
Merged

Restructure ML component #771

merged 27 commits into from
Nov 29, 2023

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Nov 27, 2023

This PR is the outcome of the refactoring carried out with @paolo-viceconte
In detail, we implemented MANN::generateDummyMANNOutput and MANN::generateDummyMANNInput in the ML component. Additionally, we restructured MANNAutoregressive for improved reset handling by introducing the MANNFootState class and consolidating reset-related quantities in MANNAutoregressive::AutoregressiveState. This simplifies the generation of the initial AutoregressiveState required to start trajectory generation. Concurrently, we restructured MANNTrajectoryGenerator by eliminating the need to pass foot positions in setInitialState. We enhanced MANNTrajectoryGenerator with reset capabilities similar to MANNAutoregressive and corrected the position and time scaling to ensure accurate scaling of the CoM, base, and feet positions. The Python bindings have been updated accordingly.

Last but not least I added two examples that show how to use MANNAutoregressive and MANNTrajectoryGenerator

MANNAutoregressive

Screencast.from.27-11-2023.18.16.54.webm

MANNTrajectoryGenerator

Screencast.from.27-11-2023.18.19.00.webm

- define the MANNFootState class
- move all the quantities required to reset the state in MANNAutoregressive::AutoregressiveState
- simplify the generation of the initial AutoregressiveState required to start the trajectory generation
- eliminate the need to pass foot positions in MANNTrajectoryGenerator::setInitialState
- enhance MANNTrajectoryGenerator with reset capabilities similar to MANNAutoregressive
- correct the position and time scaling of MANNTrajectoryGenerator to accurately scale the CoM, base, and feet positions
- update the Python bindings
@GiulioRomualdi GiulioRomualdi self-assigned this Nov 27, 2023
@GiulioRomualdi GiulioRomualdi marked this pull request as ready for review November 27, 2023 15:59
@GiulioRomualdi GiulioRomualdi enabled auto-merge (squash) November 27, 2023 16:00
Copy link
Member

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

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

Some preliminary comments

examples/MANN/MANNAutoregressive.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNAutoregressive.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNTrajectoryGenerator.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNTrajectoryGenerator.ipynb Outdated Show resolved Hide resolved
Copy link
Member

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

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

Reading just the header, I was a bit puzzled by the word dummy. I interpret dummy usually as a placeholder, so it is not clear to me why you want to generate a dummy? Can dummy be removed is the resulting output is actually well formed?

src/ML/include/BipedalLocomotion/ML/MANN.h Outdated Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANN.h Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANN.h Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANNAutoregressive.h Outdated Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANNAutoregressive.h Outdated Show resolved Hide resolved
@S-Dafarra
Copy link
Member

Reading just the header, I was a bit puzzled by the word dummy. I interpret dummy usually as a placeholder, so it is not clear to me why you want to generate a dummy? Can dummy be removed is the resulting output is actually well formed?

Reading the cpp I understood that the dummy stuff is basically filled with zeros. At this point, I would simply suggest to specify this in the doc

Co-authored-by: Stefano Dafarra <stefano.dafarra@gmail.com>
Copy link
Contributor

@paolo-viceconte paolo-viceconte left a comment

Choose a reason for hiding this comment

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

Mainly fixed typos and added suggestions for the text of the two examples!

examples/MANN/MANNAutoregressive.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNAutoregressive.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNAutoregressive.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNAutoregressive.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNTrajectoryGenerator.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNTrajectoryGenerator.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNAutoregressive.ipynb Outdated Show resolved Hide resolved
examples/MANN/MANNAutoregressive.ipynb Outdated Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANNAutoregressive.h Outdated Show resolved Hide resolved
src/ML/include/BipedalLocomotion/ML/MANNAutoregressive.h Outdated Show resolved Hide resolved
GiulioRomualdi and others added 5 commits November 28, 2023 12:58
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
Co-authored-by: Stefano Dafarra <stefano.dafarra@gmail.com>
Co-authored-by: Stefano Dafarra <stefano.dafarra@gmail.com>
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
GiulioRomualdi and others added 6 commits November 28, 2023 13:02
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
@GiulioRomualdi
Copy link
Member Author

In ae62976 I improved the documentation regarding the dummy

@GiulioRomualdi
Copy link
Member Author

Hi @paolo-viceconte and @S-Dafarra I removed the hardcoded size of the vectors in MANN e19b84e

GiulioRomualdi and others added 6 commits November 28, 2023 17:23
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
Co-authored-by: Paolo Viceconte <paolomariaviceconte@gmail.com>
@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Nov 29, 2023

Hi @S-Dafarra and @paolo-viceconte if you are fine with the changes we can merge it

@GiulioRomualdi GiulioRomualdi merged commit 83c8257 into master Nov 29, 2023
15 checks passed
@GiulioRomualdi GiulioRomualdi deleted the ml_restructuring branch November 29, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants