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

Implement NamedTuple and the CentroidalDynamics classes #642

Merged
merged 8 commits into from
Apr 6, 2023

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Mar 30, 2023

This PR implements the NamedTuple class in the System component. I took the implementation from here: https://github.com/GiulioRomualdi/named_tuple

The following classes have been modified to be compliant with named_tuple

  • DynamicalSystem
  • FixedBaseDynamics
  • LinearTimeInvariantSystem
  • FloatingBaseSystemKinematics
  • ForwardEuler
  • FirstOrderSmoother

Moreover, this PR implements the CentroidalDynamics class

@GiulioRomualdi GiulioRomualdi force-pushed the centroidal_dynamics branch 2 times, most recently from a555a30 to ad12b77 Compare April 1, 2023 13:43
@GiulioRomualdi GiulioRomualdi changed the title [TODO] Implement the first version of tagged tuple Implement NamedTuple and the CentroidalDynamics classes Apr 1, 2023
@GiulioRomualdi GiulioRomualdi force-pushed the centroidal_dynamics branch 2 times, most recently from 6805229 to 8f203bd Compare April 2, 2023 22:01
@GiulioRomualdi GiulioRomualdi self-assigned this Apr 2, 2023
@GiulioRomualdi GiulioRomualdi marked this pull request as ready for review April 3, 2023 12:40
@S-Dafarra
Copy link
Member

Meta comment. Is System a good place for NamedTuple. I think it may become useful also outside the System component. What about GenericContainer?

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 super preliminary comments. I haven't gone through the code yet

src/System/include/BipedalLocomotion/System/NamedTuple.h Outdated Show resolved Hide resolved
src/System/include/BipedalLocomotion/System/NamedTuple.h Outdated Show resolved Hide resolved
@GiulioRomualdi
Copy link
Member Author

Meta comment. Is System a good place for NamedTuple. I think it may become useful also outside the System component. What about GenericContainer?

Deal!

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 additional comments about NamedTuple (that I really like, btw)

src/System/include/BipedalLocomotion/System/NamedTuple.h Outdated Show resolved Hide resolved
src/System/include/BipedalLocomotion/System/NamedTuple.h Outdated Show resolved Hide resolved
src/System/include/BipedalLocomotion/System/NamedTuple.h Outdated Show resolved Hide resolved
src/System/include/BipedalLocomotion/System/NamedTuple.h Outdated Show resolved Hide resolved
@GiulioRomualdi
Copy link
Member Author

I moved named_tuple in GenericContainer

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.

I don't have other comments on the remaining part of the PR

@GiulioRomualdi GiulioRomualdi force-pushed the centroidal_dynamics branch 2 times, most recently from 0086ffd to 1af797e Compare April 6, 2023 08:17
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.

Just some documentation typos

The following classes have been modified:
- DynamicalSystem
- FixedBaseDynamics
- LinearTimeInvariantSystem
- FloatingBaseSystemKinematics
- ForwardEuler
- FirstOrderSmoother
…only if FRAMEWORK_COMPILE_ContinuousDynamicalSystem
@GiulioRomualdi
Copy link
Member Author

I applied your changes and I rebased the code. I will wait for the CI and I will merge it

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.

2 participants