pydrake multibody: Add bindings for PlanarJoint #13813
Conversation
EricCousineau-TRI
added
the
status: single reviewer ok
There was a problem hiding this comment.
+@EricCousineau-TRI for review
+(status: single reviewer ok)
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
bindings/pydrake/multibody/test/plant_test.py, line 1319 at r1 (raw file):
set_angle = 3. joint.set_translation(context=context, p_FoMo_F=set_translation)
BTW I love reading this as FOMO :P
There was a problem hiding this comment.
Thanks Eric! Don't merge before #13747 does.
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
bindings/pydrake/multibody/test/plant_test.py, line 1319 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
BTW I love reading this as FOMO :P
🤣
There was a problem hiding this comment.
Reviewed 2 of 6 files at r1.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mpetersen94)
bindings/pydrake/multibody/tree_py.cc, line 343 at r1 (raw file):
py::arg("context"), py::arg("theta_dot"), cls_doc.set_angular_velocity.doc) .def("set_random_pose_distribution",
I believe this is not tested.
Would it be possible to add a brief test?
FWIW It looks like none of the set_random* methods are tested :(
I won't ask ya to do those here, but could you do this one?
multibody/tree/planar_joint.h, line 2 at r1 (raw file):
#pragma once
Hm... This looks like it needs a rebase?
There was a problem hiding this comment.
Oh, soz, I thought it had already merged. Will add a blocking comment to that effect.
Reviewed 1 of 6 files at r1.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mpetersen94)
a discussion (no related file):
Requires merge of #13747.
bindings/pydrake/multibody/test/plant_test.py, line 1321 at r1 (raw file):
p_FoMo_F=set_translation) self.assertEqual( len(joint.get_translation(context=context)), 2)
This comparison seems like "asymmetric information loss" given that you set it above.
Can you use numpy_compare.assert_float_equal for testing all accessors / mutators?
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mpetersen94)
multibody/tree/planar_joint.h, line 2 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Hm... This looks like it needs a rebase?
OK Resolve this since the "Requires merge" discussion is above.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mpetersen94)
bindings/pydrake/multibody/test/plant_test.py, line 1321 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
This comparison seems like "asymmetric information loss" given that you set it above.
Can you use
numpy_compare.assert_float_equalfor testing all accessors / mutators?
OK Er, just realized that the other surrounding tests don't do that either... NVM! Feel free to do it if you feel so inclined, but I'm dismissing this comment.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mpetersen94)
bindings/pydrake/multibody/test/plant_test.py, line 1321 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
OK Er, just realized that the other surrounding tests don't do that either... NVM! Feel free to do it if you feel so inclined, but I'm dismissing this comment.
FWIW I think it should also be assert_equal(...), where the type compared is converted to T.
|
I've filed #13817 as an example for both the Would you be able to review that, then once it merges, you rebase on top of that? |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mpetersen94)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Requires merge of #13747.
Done.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mpetersen94)
bindings/pydrake/multibody/test/plant_test.py, line 1319 at r1 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
🤣
BTW I'd be afraid to take the default!
|
Can do! |
mpetersen94
force-pushed
the
bind/planar_joint
branch
from
8d5250c to
96386f4
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
a discussion (no related file):
Requires #13817 to merge first.
bindings/pydrake/multibody/tree_py.cc, line 343 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I believe this is not tested.
Would it be possible to add a brief test?
FWIW It looks like none of the
set_random*methods are tested :(
I won't ask ya to do those here, but could you do this one?
Done.
bindings/pydrake/multibody/test/plant_test.py, line 1321 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
FWIW I think it should also be
assert_equal(...), where the type compared is converted toT.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
bindings/pydrake/multibody/tree_py.cc, line 321 at r3 (raw file):
Vector3<double>>(), py::arg("name"), py::arg("frame_on_parent"), py::arg("frame_on_child"), py::arg("damping"), cls_doc.ctor.doc)
BTW, most (all?) of the other joints have a default value of 0 for damping. Would probably be good to include that here too.
mpetersen94
force-pushed
the
bind/planar_joint
branch
from
96386f4 to
ca71de0
Compare
mpetersen94
force-pushed
the
bind/planar_joint
branch
from
ca71de0 to
0b624c0
Compare
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI)
a discussion (no related file):
Previously, mpetersen94 (Mark Petersen) wrote…
Requires #13817 to merge first.
Done.
bindings/pydrake/multibody/tree_py.cc, line 321 at r3 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
BTW, most (all?) of the other joints have a default value of
0fordamping. Would probably be good to include that here too.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status:
bindings/pydrake/multibody/test/plant_test.py, line 1327 at r4 (raw file):
self.assertEqual(len(joint.damping()), 3) set_translation = array_T([1., 2.]) set_angle = T(3.)
BTW This variable may be better defined as "set_point" given that's what the others are.
There was a problem hiding this comment.
Reviewable status:
bindings/pydrake/multibody/test/plant_test.py, line 1327 at r4 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
BTW This variable may be better defined as "set_point" given that's what the others are.
All the other joints have only one "set_point" value but since this one requires two, I decided to be clearer on what each one was for.
There was a problem hiding this comment.
Reviewable status:
bindings/pydrake/multibody/test/plant_test.py, line 1327 at r4 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
All the other joints have only one "set_point" value but since this one requires two, I decided to be clearer on what each one was for.
OK Ah, gotcha. It's just that set_angle sounds like an action, whereas set_point sounds like a noun. I think set_point_{rotation,translation} would've been a bit better (or even just setpoint).
But yeah, this works!
|
Skipping slow builds |
Follow up to #13747. Adds the bindings for Planar Joint.
Relates #12404
This change is