Skip to content

Conversation

@Meow404
Copy link
Collaborator

@Meow404 Meow404 commented Jul 17, 2025

changes:

  • Add github action to report coverage
  • Improve coverage for C3 (atleast 90%)
  • Add code formatting check
  • Updated Readmes
  • Succesfully build and install python wheel

This change is Reviewable

@Meow404 Meow404 self-assigned this Jul 17, 2025
@Meow404 Meow404 linked an issue Jul 17, 2025 that may be closed by this pull request
4 tasks
@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch 2 times, most recently from e293701 to b872112 Compare July 17, 2025 15:44
@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch from b872112 to 690c138 Compare July 17, 2025 15:52
@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch 2 times, most recently from 179bf1d to 0b94277 Compare July 17, 2025 17:06
@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch from 0b94277 to e7675d8 Compare July 17, 2025 17:22
@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch 5 times, most recently from 9f9085a to 379e550 Compare July 17, 2025 18:15
@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch 5 times, most recently from acc337a to 45fc4d5 Compare July 18, 2025 15:59
@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch from 45fc4d5 to 9de7756 Compare July 18, 2025 16:01
@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch from 979e2f0 to 5f56209 Compare July 19, 2025 01:08
@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch from f2d20d4 to 00e8a99 Compare July 21, 2025 15:04
@xuanhien070594
Copy link
Contributor

core/c3.h line 203 at r2 (raw file):

   * algorithm for the first trajectory (index 0).
   */
  std::vector<Eigen::VectorXd> GetWarmStartDelta() const {

Do we use those function any where? If not, we can just remove them.

@xuanhien070594
Copy link
Contributor

core/c3.h line 213 at r2 (raw file):

   * algorithm for the first trajectory (index 0).
   */
  std::vector<Eigen::VectorXd> GetWarmStartBinary() const {

Same comment as above.

@xuanhien070594
Copy link
Contributor

examples/c3_example.cc line 187 at r2 (raw file):

    /// print the state
    // std::cout << "state: " << x[i + 1] << std::endl;

We can add a flag verbose for printing out state.

@xuanhien070594
Copy link
Contributor

bindings/pyc3/README.md line 38 at r2 (raw file):

```sh
bazel build //bindings/pyc3:pyc3_wheel

I have a question: will bazel use the python version in the virtual environment or the system python?

@xuanhien070594
Copy link
Contributor

bindings/pyc3/README.md line 65 at r2 (raw file):

Complete examples are available in the `examples/python/` directory:

- **`c3_example.py`**: Basic C3 optimization example

Suggestion:

- **`c3_example.py`**: Basic C3 with toy examples

@xuanhien070594
Copy link
Contributor

README.md line 5 at r2 (raw file):

[**Main Paper: Consensus Complementarity Control**](https://arxiv.org/abs/2304.11259)

<p align="center">

Nit. Can we make the build status and coverage status on the same line?

@xuanhien070594
Copy link
Contributor

README.md line 22 at r2 (raw file):

```shell
cd c3
bazel build ...

Can we add instruction to install bazel or bazelisk

@xuanhien070594
Copy link
Contributor

README.md line 120 at r2 (raw file):

├── multibody/         # algorithms for computation in multibody environments
├── third_party/       # External dependencies
└── WORKSPACE          # Bazel workspace file

We don't have WORKSPACE file.

@xuanhien070594
Copy link
Contributor

README.md line 126 at r2 (raw file):

## Reference

For a detailed explanation of the C3 algorithm, please refer to the [main paper](https://arxiv.org/abs/2304.11259). Additional resources and in-depth examples can be found in the [dairlib repository](https://github.com/DAIRLab/dairlib).

We might consider to add a Citing C3 section where we'll put the BibTeX entry for C3 paper.

@xuanhien070594
Copy link
Contributor

examples/README.md line 3 at r2 (raw file):

# C3 Standalone Example
This example (c3_example.cc) demonstrates how to set up and solve C3 problems for different systems (cartpole, finger gaiting, and pivoting) using only Eigen and the core C3 classes, without Drake dependencies. It is useful for understanding the core algorithm and for running C3 on custom LCS models.

Core C3 is actually using Drake dependencies (e.g MathematicalProgram). We can say in those examples, we derive analytical dynamics equations instead of involving Drake's MultibodyPlant.

@xuanhien070594
Copy link
Contributor

examples/README.md line 40 at r2 (raw file):

# LCS Factory System Example

The cartpole with softwalls problem is modeled as a Linear Complementarity System (LCS) generated by the LCS factory. This LCS forms the basis for running the C3 controller, which stabilizes the cartpole while tracking predefined state trajectories.

We can add more a little bit more details such as: The LCS model provides a piecewise-affine approximation of the nonlinear dynamics, generated using the LCS factory.

@xuanhien070594
Copy link
Contributor

examples/README.md line 64 at r2 (raw file):

#### Cube Pivoting
```sh
# Run the example

We don't need comment here.

@xuanhien070594
Copy link
Contributor

examples/README.md line 53 at r2 (raw file):

### Build the example
```sh
# Build the cartpole LCS factory system example

We also don't need comment here.

@xuanhien070594
Copy link
Contributor

examples/README.md line 75 at r2 (raw file):

C3 Standalone Example (Python)
```sh
# Build and run the Python C3 standalone example

Nit. I think the comments seem to be redundant.

@xuanhien070594
Copy link
Contributor

core/test/core_test.cc line 358 at r2 (raw file):

  ASSERT_TRUE(delta.size() > 0);

  auto binary = optimizer.GetWarmStartBinary();

Are we using these functions GetWarmStartDelta or GetWarmStartBinary any where? If not, I think we can remove them.

@xuanhien070594
Copy link
Contributor

systems/test/systems_test.cc line 328 at r2 (raw file):

}

TEST_F(LCSFactorySystemTest, OutputContactJacobianIsValid) {

Note that after merging #10, this test is no longer valid. We'll need a new test.

Copy link
Contributor

@xuanhien070594 xuanhien070594 left a comment

Choose a reason for hiding this comment

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

Only have some minor comments. Please note that some of the unit tests for LCSFactorySystem and LCSFactory will be obsolete after merging #10

Reviewed 13 of 22 files at r1, 45 of 45 files at r2, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @Meow404)

@Meow404
Copy link
Collaborator Author

Meow404 commented Jul 28, 2025

Only have some minor comments. Please note that some of the unit tests for LCSFactorySystem and LCSFactory will be obsolete after merging #10

Reviewed 13 of 22 files at r1, 45 of 45 files at r2, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @Meow404)

Yeah, I'd want to try and merge this PR first and then add/fix unit tests in #10 after a rebase.

Copy link
Collaborator Author

@Meow404 Meow404 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @xuanhien070594)


README.md line 22 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

Can we add instruction to install bazel or bazelisk

Done.


README.md line 120 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

We don't have WORKSPACE file.

Done.


README.md line 126 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

We might consider to add a Citing C3 section where we'll put the BibTeX entry for C3 paper.

Done.


bindings/pyc3/README.md line 38 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

I have a question: will bazel use the python version in the virtual environment or the system python?

My understanding is it will use the version specified in the .bazelrc file


core/c3.h line 203 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

Do we use those function any where? If not, we can just remove them.

Done.


core/c3.h line 213 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

Same comment as above.

Done.


examples/c3_example.cc line 187 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

We can add a flag verbose for printing out state.

Done.


examples/README.md line 3 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

Core C3 is actually using Drake dependencies (e.g MathematicalProgram). We can say in those examples, we derive analytical dynamics equations instead of involving Drake's MultibodyPlant.

Done.


examples/README.md line 40 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

We can add more a little bit more details such as: The LCS model provides a piecewise-affine approximation of the nonlinear dynamics, generated using the LCS factory.

Done.


examples/README.md line 53 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

We also don't need comment here.

Done.


examples/README.md line 64 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

We don't need comment here.

Done.


systems/lcs_factory_system.cc line 3 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

We don't need iostream right?

Done.


systems/test/systems_test.cc line 328 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

Note that after merging #10, this test is no longer valid. We'll need a new test.

Hoping to merge this PR first, then resolve the dependencies and coverage in #10


core/test/core_test.cc line 358 at r2 (raw file):

Previously, xuanhien070594 (Hien Bui) wrote…

Are we using these functions GetWarmStartDelta or GetWarmStartBinary any where? If not, I think we can remove them.

Done.


bindings/pyc3/README.md line 65 at r2 (raw file):

Complete examples are available in the `examples/python/` directory:

- **`c3_example.py`**: Basic C3 optimization example

Done.

@Meow404 Meow404 requested a review from xuanhien070594 July 28, 2025 20:51
@xuanhien070594
Copy link
Contributor

README.md line 154 at r3 (raw file):

      primaryClass={cs.RO},
      url={https://arxiv.org/abs/2304.11259}, 
}

Suggestion:

@article{Aydinoglu2024,
  title = {Consensus Complementarity Control for Multi-Contact MPC},
  author = {Aydinoglu, Alp and Wei, Adam and Huang, Wei-Cheng and Posa, Michael},
  year = {2024},
  month = jul,
  journal = {IEEE Transactions on Robotics (TRO)},
  youtube = {L57Jz3dPwO8},
  arxiv = {2304.11259},
  doi = {10.1109/TRO.2024.3435423},
  url = {https://ieeexplore.ieee.org/document/10614849}
}

xuanhien070594
xuanhien070594 previously approved these changes Jul 28, 2025
Copy link
Contributor

@xuanhien070594 xuanhien070594 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Only need to change the citation BibTeX of C3 paper.

Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Meow404)

@Meow404 Meow404 force-pushed the stephen/standardize-C3-repo branch from e64ed7a to 1ee8847 Compare July 28, 2025 21:13
Copy link
Contributor

@xuanhien070594 xuanhien070594 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Meow404)

@Meow404 Meow404 merged commit 13ba28d into main Jul 28, 2025
2 of 5 checks passed
@Meow404 Meow404 deleted the stephen/standardize-C3-repo branch July 28, 2025 21:33
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.

Standardize C3 repo

2 participants