van_der_pol: Add self-contained example Python bindings #9648
Conversation
There was a problem hiding this comment.
+@jwnimmer-tri for review / evaluation of worthwhile-ness.
Reviewable status: all discussions resolved, platform LGTM missing
EricCousineau-TRI
force-pushed
the
feature/py_van_der_pol_in_tree
branch
from
3bac6f7 to
d3b127e
Compare
There was a problem hiding this comment.
fyi - the major (only) consumer of this library outside of drake that I know about is:
https://github.com/RussTedrake/underactuated/tree/master/src/van_der_pol
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing
|
(And the reason it’s c++ instead of pure python is because i need transmogrification for the trajectory optimization) |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, platform LGTM missing
examples/van_der_pol/BUILD.bazel, line 89 at r1 (raw file):
"van_der_pol_py.cc", ], py_deps = ["//bindings/pydrake"],
Can't we be much more granular than this? In my dev branch, i made dependencies directly to the library artifacts that i depended on in pydrake (but did have to increase their visibility to achieve this). But it was very satisfying to have minimal compilation steps between runs!
EricCousineau-TRI
force-pushed
the
feature/py_van_der_pol_in_tree
branch
from
d3b127e to
f503576
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, platform LGTM missing
examples/van_der_pol/BUILD.bazel, line 89 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Can't we be much more granular than this? In my dev branch, i made dependencies directly to the library artifacts that i depended on in pydrake (but did have to increase their visibility to achieve this). But it was very satisfying to have minimal compilation steps between runs!
Done.
There was a problem hiding this comment.
i can't review the bazel details, but the default_visibility and vanderpol look good to me.
Reviewed 3 of 10 files at r1, 17 of 17 files at r3.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM from [russtedrake]
bindings/pydrake/systems/BUILD.bazel, line 127 at r3 (raw file):
"//bindings/pydrake:__subpackages__", "//examples:__subpackages__", ],
nit: why not just remove this (if it's the default)?
examples/van_der_pol/BUILD.bazel, line 61 at r3 (raw file):
# TODO(eric.cousineau): Try to hide more of these details for authoring Python # binding examples.
btw -- yes please.
There was a problem hiding this comment.
just fwiw, because the default visibility changes are in here, my next set of PRs is waiting on this one.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM from [russtedrake]
|
I don't think this PR needs to be the place that changes visibility. (This PR is going to take a while.) If you need visibility changes for your PR, just make them there. |
There was a problem hiding this comment.
What's the priority on this?
@EricCousineau-TRI I think this is moving broadly in the right direction. I think, before we promulgate this as the answer for pydrake example bindings, we would want to tidy up the boilerplate -- at minimum in areas that are actually risky / important, such as the ODR-related spellings.
The other question is whether we want to allow downstream users to do from van_der_pol import VanDerPolOscillator or not. We probably need to consider which python module this lives within.
What's a good next step -- should I go through a high-level first pass and note some places where a little design rework is needed?
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM from [russtedrake]
|
Given our prior discussions, I'll set this as low priority; we can address it after the class is done? |
EricCousineau-TRI
force-pushed
the
feature/py_van_der_pol_in_tree
branch
from
f503576 to
d50273a
Compare
|
Ok |
by including manipulation_station targets, and their dependencies, in libdrake.so. we consider this a temporary, and very imperfect, solution until in-tree examples can produce their own bindings w/o going through libdrake.so. (see RobotLocomotion#9648)
by including manipulation_station targets, and their dependencies, in libdrake.so. we consider this a temporary, and very imperfect, solution until in-tree examples can produce their own bindings w/o going through libdrake.so. (see RobotLocomotion#9648)
by including manipulation_station targets, and their dependencies, in libdrake.so. we consider this a temporary, and very imperfect, solution until in-tree examples can produce their own bindings w/o going through libdrake.so. (see RobotLocomotion#9648)
by including manipulation_station targets, and their dependencies, in libdrake.so. we consider this a temporary, and very imperfect, solution until in-tree examples can produce their own bindings w/o going through libdrake.so. (see RobotLocomotion#9648)
by including manipulation_station targets, and their dependencies, in libdrake.so. we consider this a temporary, and very imperfect, solution until in-tree examples can produce their own bindings w/o going through libdrake.so. (see RobotLocomotion#9648)
by including manipulation_station targets, and their dependencies, in libdrake.so. we consider this a temporary, and very imperfect, solution until in-tree examples can produce their own bindings w/o going through libdrake.so. (see RobotLocomotion#9648)
|
Closing for now, pending resolution of #9645. |
This is an initial idea of what an example with bindings could look like post-#9645.
Admittedly,
van_der_polcould be re-implemented in pure Python.If this doesn't have value in the larger of scope of #9646 w.r.t. bindings, then I can either close this PR or we can re-implement it in pure Python.
\cc @RussTedrake
This change is