Skip to content
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 preliminary Python bindings and examples for systems framework. #7590

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Dec 12, 2017

This is the initial binding of a small portion of the Systems framework in Python.

Introduces:

  • Simplified diagram similar to that in ExampleDiagram (//systems/framework:diagram_test)
  • Simple example to plot the graphviz image of a system, if pydot is installed.
  • Simple example of implementing a custom system, and simulating said system
  • Unittest indicating lifetime limitations using unique_ptr (to concretely track capabilities, if/when we decide to switch to shared_ptrs)

Caveats at present:

Tasks:


This change is Reviewable

@EricCousineau-TRI EricCousineau-TRI changed the title [WIP] Add preliminary bindings and examples for systems framework. [WIP] Add preliminary Python bindings and examples for systems framework. Dec 12, 2017
@EricCousineau-TRI EricCousineau-TRI force-pushed the feature/py_systems_pr1 branch 2 times, most recently from ec458cf to 005e836 Compare December 13, 2017 15:32
@EricCousineau-TRI EricCousineau-TRI changed the title [WIP] Add preliminary Python bindings and examples for systems framework. Add preliminary Python bindings and examples for systems framework. Dec 20, 2017
@EricCousineau-TRI
Copy link
Contributor Author

+@jadecastro for feature review (if you're comfortable reviewing the Python bindings)
+@ggould-tri for platform review, please.


Review status: 0 of 16 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please.

@ggould-tri
Copy link
Contributor

:lgtm: pending notes below.


Reviewed 16 of 16 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


bindings/pydrake/systems/BUILD.bazel, line 29 at r1 (raw file):

        "//drake/bindings/pydrake:common_py",
        # TODO(eric.cousineau): This does NOT include direct dependencies to
        # submodule. This is to preserve C++-like namespaces.

BTW It is not clear to me what this means or what remains to do here.


bindings/pydrake/systems/drawing.py, line 4 at r1 (raw file):

# Provides general visualization utilities. This is NOT related to `rendering`.
# @note This is an optional module, dependent on `pydot` and `matplotlib` being
# installed.

Is this a special pybind11 style of docstring? Some sort of passthrough to doxygen?
I don't quite understand this.


bindings/pydrake/systems/framework_py.cc, line 60 at r1 (raw file):

    using Base = py::wrapper<System<T>>;
    using Base::Base;
    // Expose protected methods.

This seems dangerous. Do we document anywhere the pitfalls that this might create? Alternatively, if we are happy with DeclareInputPort being public here, why not also in the superclass?

Maybe it would make sense to export it with a _-prefixed name as is conventional in python for access-restricted fields. This might at least force non-subclass users to fight the linter.


bindings/pydrake/systems/primitives_py.cc, line 35 at r1 (raw file):

  py::class_<ZeroOrderHold<T>, LeafSystem<T>>(m, "ZeroOrderHold")
    .def(py::init<double, int>());
}

Why only these? I suspect at least that anyone trying to use this will find themselves needing Multiplexer or Gain. You should at least TODO that some systems are missing here so people will know whether or not it is intentional and whether they should just add anything they need.


bindings/pydrake/systems/test/custom_test.py, line 41 at r1 (raw file):

            sum += input_vector.get_value()
        # TODO(eric.cousineau): Remove this when we ensure that we can mutate
        # vectors. Not sure why this isn't working.

FYI -- This sounds like an issue that should have a bug number; consider filing it as a bug.


bindings/pydrake/systems/test/general_test.py, line 75 at r1 (raw file):

        diagram = builder.Build()
        # TODO(eric.cousineau): Figure out simple unicode handling, if
        # necessary. (Restore the snowman.)

FYI nobody is going to know what you mean by this, including probably you if you get back to this a year later.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

Review status: 10 of 18 files reviewed at latest revision, 6 unresolved discussions.


bindings/pydrake/systems/BUILD.bazel, line 29 at r1 (raw file):

Previously, ggould-tri wrote…

BTW It is not clear to me what this means or what remains to do here.

Done.


bindings/pydrake/systems/drawing.py, line 4 at r1 (raw file):

Previously, ggould-tri wrote…

Is this a special pybind11 style of docstring? Some sort of passthrough to doxygen?
I don't quite understand this.

Done. Nope, just me thinking in Bash mode.


bindings/pydrake/systems/framework_py.cc, line 60 at r1 (raw file):

Previously, ggould-tri wrote…

This seems dangerous. Do we document anywhere the pitfalls that this might create? Alternatively, if we are happy with DeclareInputPort being public here, why not also in the superclass?

Maybe it would make sense to export it with a _-prefixed name as is conventional in python for access-restricted fields. This might at least force non-subclass users to fight the linter.

Done. Following PEP8 - thanks for catching that!


bindings/pydrake/systems/primitives_py.cc, line 35 at r1 (raw file):

Previously, ggould-tri wrote…

Why only these? I suspect at least that anyone trying to use this will find themselves needing Multiplexer or Gain. You should at least TODO that some systems are missing here so people will know whether or not it is intentional and whether they should just add anything they need.

Done. (Added TODO.)


bindings/pydrake/systems/test/custom_test.py, line 41 at r1 (raw file):

Previously, ggould-tri wrote…

FYI -- This sounds like an issue that should have a bug number; consider filing it as a bug.

Done. (Just hadn't returned the right thing.)


bindings/pydrake/systems/test/general_test.py, line 75 at r1 (raw file):

Previously, ggould-tri wrote…

FYI nobody is going to know what you mean by this, including probably you if you get back to this a year later.

Done.


Comments from Reviewable

@ggould-tri
Copy link
Contributor

Nice; I like the new test especially.


Reviewed 8 of 8 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please.

@ggould-tri
Copy link
Contributor

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jadecastro
Copy link
Contributor

Pass complete.


Reviewed 15 of 16 files at r1, 4 of 8 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


bindings/pydrake/systems/drawing.py, line 1 at r1 (raw file):
BTW, missing header?

#!/usr/bin/env python


bindings/pydrake/systems/drawing.py, line 25 at r1 (raw file):

    g = pydot.graph_from_dot_data(dot_text)
    if isinstance(g, list):
        # @see Ioannis's follow-up comment.

BTW, rather than this, you may as well consider just summarizing the gist of this comment, specific to what's in the code below.


bindings/pydrake/systems/framework_py.cc, line 7 at r2 (raw file):

#include <pybind11/stl.h>

#include "drake/common/nice_type_name.h"

BTW, Is this unused?


bindings/pydrake/systems/test/custom_test.py, line 81 at r2 (raw file):

        simulator.StepTo(1)
        # Ensure that we have the outputs we want.
        state = diagram.GetMutableSubsystemState(zoh, context)

Since Simulator does not own the Diagram context, does it now share it? If so, shall a test be added to test the equivalent of simulator.get_mutable_context()?


bindings/pydrake/systems/test/graphviz_example.py, line 25 at r1 (raw file):

plot_system_graphviz(diagram)
plt.show()

BTW, Is there any tests to be checked here? Or is this purely for tutorial value?


Comments from Reviewable

@EricCousineau-TRI EricCousineau-TRI force-pushed the feature/py_systems_pr1 branch 2 times, most recently from ccebec4 to 9939c92 Compare December 21, 2017 16:49
@ggould-tri
Copy link
Contributor

Reviewed 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


bindings/pydrake/systems/drawing.py, line 1 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, missing header?

#!/usr/bin/env python

That doesn't seem right to me. There's no main in here, so it's not executable and a shebang line would just confuse matters.


Comments from Reviewable

@jadecastro
Copy link
Contributor

Review status: all files reviewed at latest revision, 4 unresolved discussions.


bindings/pydrake/systems/drawing.py, line 1 at r1 (raw file):

Previously, ggould-tri wrote…

That doesn't seem right to me. There's no main in here, so it's not executable and a shebang line would just confuse matters.

I see. Thanks.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please.
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please.

@EricCousineau-TRI
Copy link
Contributor Author

Review status: 17 of 18 files reviewed at latest revision, 5 unresolved discussions.


bindings/pydrake/systems/drawing.py, line 1 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

I see. Thanks.

BTW Would you be able to mark yourself as satisfied?


bindings/pydrake/systems/drawing.py, line 25 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, rather than this, you may as well consider just summarizing the gist of this comment, specific to what's in the code below.

Done.


bindings/pydrake/systems/framework_py.cc, line 7 at r2 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, Is this unused?

Done.


bindings/pydrake/systems/test/custom_test.py, line 81 at r2 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

Since Simulator does not own the Diagram context, does it now share it? If so, shall a test be added to test the equivalent of simulator.get_mutable_context()?

Done. Added this into general_test.
And actually, the simulator does own the context, at this point.


bindings/pydrake/systems/test/graphviz_example.py, line 25 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, Is there any tests to be checked here? Or is this purely for tutorial value?

It's just for tutorial value. I've added a TODO in BUILD.bazel to have this be tested.
(I'm meta-testing plotting with matplotlib on CI with #7657).


Comments from Reviewable

@ggould-tri
Copy link
Contributor

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@jadecastro
Copy link
Contributor

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants