-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add pydrake bindings for model directives. #14144
Add pydrake bindings for model directives. #14144
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.
+@EricCousineau-TRI
Follow-up to previous PR: Manual port of python bindings.
I am not a bindings expert and expect you will have plenty of objections here.
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)
1af8df6
to
9c10b90
Compare
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.
with a couple of nits - thanks!
I'm fine with serving as both feature and platform
Reviewed 4 of 4 files at r1.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @ggould-tri)
bindings/pydrake/multibody/parsing_py.cc, line 91 at r1 (raw file):
py::arg("directives"), py::arg("plant"), py::arg("parser"), (std::string(parsing_doc.ProcessModelDirectives.doc) + "\nNOTE: pydrake does not support `ModelWeldErrorFunction`.")
nit This should be compatible with Sphinx Napoleon:
https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html#docstring-sections
See example for a warning directive used in Drake here:
drake/bindings/pydrake/systems/lcm_py.cc
Lines 72 to 80 in ae06238
constexpr char kLcmInterfaceSystemClassWarning[] = R"""( | |
Warning: | |
In C++, this class inherits both LeafSystem and DrakeLcmInterface. However, | |
in Python, this only inherits from LeafSystem since our fork of pybind11 | |
does not support multiple inheritance. Additionally, it only exposes the | |
constructor accepting a DrakeLcmInterface, so that it can still be used in | |
interface code. | |
)"""; |
See GSG styling (though not very comprehensive for directives...):
https://drake.mit.edu/styleguide/pyguide.html
(Will make a PR to try and link that in)
multibody/parsing/BUILD.bazel, line 272 at r1 (raw file):
data = glob(["test/process_model_directives_test/**"]), visibility = [ "//visibility:public",
nit //visibility:public
means other downstream consumer (e.g. Anzu, etc) can use this.
Consider the following options:
//:__subpackages__
- Drake only (slightly more conservative)//bindings/pydrake:__subpackages__
- Bindings only (more conservative)//bindings/pydrake:multibody:__pkg__
- Multibody bindings only (most conservative).
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.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @ggould-tri)
bindings/pydrake/multibody/parsing_py.cc, line 67 at r1 (raw file):
py::class_<parsing::ModelDirectives>( m, "ModelDirectives", parsing_doc.ModelDirectives.doc); m.def("LoadModelDirectives", &parsing::LoadModelDirectives,
All of these bindings should have docstrings available.
Can you add them in everywhere?
bindings/pydrake/multibody/parsing_py.cc, line 69 at r1 (raw file):
m.def("LoadModelDirectives", &parsing::LoadModelDirectives, py::arg("filename")); py::class_<parsing::ModelInstanceInfo>(m, "ModelInstanceInfo")
nit Per above defect, it'll be simpler if you just do a per-class alias:
Please consider aliasing using the provided template:
https://drake.mit.edu/doxygen_cxx/group__python__bindings.html#PydrakeDoc
Currently, you're breaking existing precedents (no alias or fully aliasing), I believe.
* Weld error support omitted for simplicity. * Follow-up to RobotLocomotion#14038 * Completes RobotLocomotion#13282
9c10b90
to
5b83c18
Compare
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.
+(status: single reviewer ok)
Reviewable status: 1 unresolved discussion (waiting on @EricCousineau-TRI)
bindings/pydrake/multibody/parsing_py.cc, line 67 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
All of these bindings should have docstrings available.
Can you add them in everywhere?
Done.
bindings/pydrake/multibody/parsing_py.cc, line 69 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Per above defect, it'll be simpler if you just do a per-class alias:
Please consider aliasing using the provided template:
https://drake.mit.edu/doxygen_cxx/group__python__bindings.html#PydrakeDocCurrently, you're breaking existing precedents (no alias or fully aliasing), I believe.
Not quite sure what you mean here, but I think I have done it now?
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.
Reviewed 3 of 3 files at r2.
Reviewable status: complete! all discussions resolved, LGTM from assignee EricCousineau-TRI(platform)
bindings/pydrake/multibody/parsing_py.cc, line 69 at r1 (raw file):
Previously, ggould-tri wrote…
Not quite sure what you mean here, but I think I have done it now?
OK Yup - thanks!
bindings/pydrake/multibody/parsing_py.cc, line 88 at r2 (raw file):
constexpr char kWeldErrorDisclaimer[] = R"""( Note:
BTW Not sure if this will render well with the extra whitespace (given that it's getting appended, rather than being the docstring itself), but it's not a huge deal for me atm.
This change is