Implement derivative of fluid forces#1022
Implement derivative of fluid forces#1022SUZ-tsinghua wants to merge 8 commits intogoogle-deepmind:mainfrom
Conversation
|
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. |
|
@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! |
|
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. |
|
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.😊 |
…ellipsoid-only kernel implementation
|
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. |
|
@Aaryan-549 @SUZ-tsinghua probably makes sense to finish the review of #897 for adding the implementation of 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! |
|
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). |
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.