Skip to content

Implement derivative of fluid forces#1022

Open
SUZ-tsinghua wants to merge 8 commits intogoogle-deepmind:mainfrom
SUZ-tsinghua:dev
Open

Implement derivative of fluid forces#1022
SUZ-tsinghua wants to merge 8 commits intogoogle-deepmind:mainfrom
SUZ-tsinghua:dev

Conversation

@SUZ-tsinghua
Copy link

Hi, I implemented the derivative of fluid forces and also wrote tests for derivative and forward. It can pass all tests.
I also saw this pr #897 . My implementation might be helpful.

@google-cla
Copy link

google-cla bot commented Jan 14, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@thowell
Copy link
Collaborator

thowell commented Jan 14, 2026

@SUZ-tsinghua thank you for your contribution to mujoco warp!

please coordinate with @Aaryan-549 on this feature since it seems that there is overlap between this pr and #897. thanks!

@Aaryan-549
Copy link
Contributor

Hi @SUZ-tsinghua, thanks again. I reviewed your changes. Your test case test_smooth_vel_fluid is excellent and exactly what the maintainers requested for #897.

With your permission, I'm going to integrate your derivative_test.py and the io.py enablement into my PR so we can get the Ellipsoid feature merged. I'll stick to my kernel implementation for now to keep the scope focused on Ellipsoids (as you also included the Inertia Box fallback). I will add a Co-authored-by tag to my commit to ensure you get credit for the testing logic.

@SUZ-tsinghua
Copy link
Author

Hi @Aaryan-549, thank you. Another option is directly merging my pr, since I think my version is more complete. I can add you as a co-author for your credit.😊

Aaryan-549 added a commit to Aaryan-549/mujoco_warp that referenced this pull request Jan 14, 2026
@Aaryan-549
Copy link
Contributor

Thanks for the offer, but I don't think swapping PRs is the right move at this stage.

PR #897 has already undergone significant review iterations (30+ comments) with @thowell and is very close to merging. Switching to a new PR now would invalidate that review effort and force the maintainers to restart the process.

Also, I noticed your PR includes the Inertia Box implementation. While useful, that expands the scope beyond the original Ellipsoid feature request. It's generally safer to merge the Ellipsoid support first as a focused unit (which #897 does), and then add the Inertia Box as a separate, follow-up PR.

I’ve just updated #897 with the mj_forward parity tests (referencing your logic, thank you!) and the io.py fix. Let’s get the base Ellipsoid feature in through #897 first.

@thowell
Copy link
Collaborator

thowell commented Jan 15, 2026

@Aaryan-549 @SUZ-tsinghua
thank you for both contributing to the project!

probably makes sense to finish the review of #897 for adding the implementation of mjd_ellipsoidFluid. @SUZ-tsinghua it would great if you would add a review to that pr.

after we land #897, we can coordinate the next features. sound like a plan? otherwise, please let us know if you have alternative suggestions. thanks!

@SUZ-tsinghua
Copy link
Author

Hi @thowell , thanks for your comment. I personally prefer this pr, because this pr is more complete and cleaner. I have implemented all necessary tests and derivatives for both ellipsoid and box fluid model. I think this pr requires less review and can alleviate further effort to implement a new feature (derivative for box fluid model).
It would be nice if you could spend a little time reviewing my changes. If after little reviewing and you still think finishing #897 is better, I would be happy to review #897 😊

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.

3 participants