-
Notifications
You must be signed in to change notification settings - Fork 41
inertia box fluid model #191
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - some comments, mostly about wp.static usage.
mujoco_warp/_src/passive.py
Outdated
lvel_torque = rotT @ torque | ||
lvel_force = rotT @ force | ||
|
||
if m.opt.wind[0] or m.opt.wind[1] or m.opt.wind[2]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if there is a big benefit to this, but could be wp.max(m.opt.wind) > 0.0.
Or I guess this information is even statically available, so maybe wrap in wp.static? actually, you're not running that code right now anyway if all of them are zero, are you going to reuse this function in a different kernel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrapped in wp.static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the functionality from _inertia_box_fluid_model
is moved into box_fluid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is lovely code and nicely written, just a few nits
@@ -227,22 +227,6 @@ def test_get_data_into_m(self): | |||
np.testing.assert_allclose(mjd.qLD, mjd_ref.qLD) | |||
np.testing.assert_allclose(mjd.qM, mjd_ref.qM) | |||
|
|||
def test_option_physical_constants(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should still check for ellipsoid fluid and fail?
opt_viscosity = opt.viscosity | ||
opt_density = opt.density | ||
|
||
@nested_kernel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets a bit subjective, but I think the bar might be higher for using a nested kernel. For tiles we have no choice, but for here, we can pass the opts in as params.
If it led to a significant performance difference I would understand, but this kernel looks pretty cheap.
In particular, my understanding is that the performance hit comes from thread divergence. e.g. some threads have viscosity and others don't. I don't think that's the case here, no?
adds inertia box fluid model
https://mujoco.readthedocs.io/en/latest/computation/fluid.html#inertia-model
https://github.com/google-deepmind/mujoco/blob/d3bc0544d39c0933067e493f4c1c7ebfd15ae9f3/src/engine/engine_passive.c#L527