pydrake systems: Expose initial round of different scalar types

[EricCousineau-TRI]

Requires:

• eigen: Disable dtype=object arrays from being referenced pybind11#17 - disables any runtime usage of references for dtype=object

Can merge without, but would be nice to have:

• (probs won't get 'em in time) numpy: Require NumPy >= 0.15.0 #8608 + [WIP] Use user-defined NumPy dtypes for AutoDiffXd, Expression, etc. #8452 to remove some of the silly shims, but this can maintain functionality, while also (annoyingly) warning users when things will break if the wrong operations are done upon a type.

This does not yet enable custom Python classes with scalar-type conversion. We can hack that in if need be, or wait for the above PRs to complete that.

\cc @RussTedrake @gizatt

This change is 

[EricCousineau-TRI]

+@jwnimmer-tri for feature review, please.

While the LOC is numerically high, most of the operations are very boiler-plate-y, and relatively simple.

---

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

---

Comments from Reviewable

[jwnimmer-tri]

Jenkins has a complaint:

/framework_py_semantics.pic.o: C++ compilation of rule '//bindings/pydrake/systems:framework.so' failed (Exit 1)
bindings/pydrake/systems/framework_py_semantics.cc: In lambda function:
bindings/pydrake/systems/framework_py_semantics.cc:55:30: error: declaration of 'using T = decltype (dummy)' shadows a global declaration [-Werror=shadow]
     using T = decltype(dummy);
                              ^
bindings/pydrake/systems/framework_py_semantics.cc:28:17: note: shadowed declaration is here
 using T = double;

---

Comments from Reviewable

[jwnimmer-tri]

Checkpoint.

---

Reviewed 15 of 17 files at r1.
Review status: 15 of 17 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

bindings/pydrake/autodiffutils_py.cc, line 28 at r1 (raw file):

  py::class_<AutoDiffXd> autodiff(m, "AutoDiffXd");
  autodiff
    .def(py::init<double>())

The three new lines in this file lack direct unit test coverage?

---

bindings/pydrake/systems/framework_py_values.cc, line 27 at r1 (raw file):

  using namespace drake::systems;

  auto bind_common_scalar_types = [m](auto dummy) {

Its difficult for me to immediately understand what passing a py::module by-value means.

(Ditto for all the other type_visit lambda targets in other files.)

---

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

    DefineTemplateClassWithDefault<Multiplexer<T>, LeafSystem<T>>(
        m, "Multiplexer", GetPyParam<T>())

FYI Alpha-sort "Multiplexer" between "LinearSystem" and "Passthrough", for consistency?

---

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

  type_visit(bind_common_scalar_types, pysystems::CommonScalarPack{});

  using T = double;

Given that T is used above with special meaning (its actually a generic scalar marker), it seems misleading in the below code for T to not mean "various scalars".

I suggest just saying double literally below where needed.

---

bindings/pydrake/systems/systems_pybind.h, line 103 at r1 (raw file):

    double,
    AutoDiffXd
    >;

Either the above two lists are supposed to be identical to default_scalars.h in which case its a brittleness defect not to have cross-linked comments in the two files instructing developers to keep them in sync, or else these lists do not need to be kept in sync in which case its a confusion defect how and why these two lists should evolve differently.

---

bindings/pydrake/systems/trajectory_optimization_py.cc, line 31 at r1 (raw file):

      .def("fixed_timestep", &MultipleShooting::fixed_timestep)
      // TODO(eric.cousineau): Restore original bindings (using VectorXBlock)
      // once dtype=custom is resolved.

BTW If I'm understanding correctly, I think the important fact here is that the functions return references instead of copies. If that's what you mean, it probably should be made more obvious.

---

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

        # Quick check of instantions for given types.
        self._check_instantiations(Simulator_, False)
        self._check_instantiations(BasicVector_)

What about the VectorBase / Supervector / Subvector?

---

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

            system = Adder_[T](1, 1)
            # N.B. Current scalar conversion does not auto-register idempotent
            # conversions.

I don't understand what this is saying.

---

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

            if T != Expression:
                system_sym = system.ToSymbolic()
                self.assertIsInstance(system_sym, System_[Expression])

As a basic sanity check, it seems worth checking some fact about the resulting system, e.g. that the num_inputs == 1 and input_port[0].size == 1, or similar.

---

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

                self.assertIsInstance(system_sym, System_[Expression])

    def test_simular_ctor(self):

BTW typo

---

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

                    # possible, to use for `np.allclose`.
                    self.assertEqual(value.shape, (1,))
                    self.assertEqual(value[0], AutoDiffXd(1.))

BTW ... or else fail-fast (when T is neither of the two that its supposed to be?).

---

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

        self._check_instantiations(Saturation_)
        self._check_instantiations(SignalLogger_)
        self._check_instantiations(WrapToSystem_)

Unclear why missing:

• ConstantValueSource
• ZeroOrderHold

---

bindings/pydrake/util/cpp_template.py, line 169 at r1 (raw file):

            # Define `__qualname__` in Python2 because that's what `pybind11`
            # uses when showing function signatures when an overload cannot be
            # found.

FYI Should there be a unit test to make any assertions about the quality of the exception messages that are coming out related to this?

---

tools/workspace/pybind11/repository.bzl, line 9 at r1 (raw file):

# PR DRAFT(eric.cousineau): Change this once RobotLocomotion/pybind11#17 lands.
_COMMIT = "f8d6b820a30936d21f261e2162464f1cabe4ceda"

Blocking on reaching our master.

---

Comments from Reviewable

[jwnimmer-tri]

First pass complete.

---

Reviewed 2 of 17 files at r1.
Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

---

bindings/pydrake/systems/framework_py_semantics.cc, line 52 at r1 (raw file):

  BindTypeSafeIndex<AbstractParameterIndex>(m, "AbstractParameterIndex");

  using AbstractValuePtrList = vector<unique_ptr<AbstractValue>>;

I can't tease out why this moved, especially when BasicVectorPtrList didn't move. The prior location down below seems to be to be closer to its first point of use, which is more clear to the reader.

---

bindings/pydrake/systems/framework_py_semantics.cc, line 149 at r1 (raw file):

  // TODO(eric.cousineau): Figure out how to handle template-specialized method
  // signatures(e.g. GetValue<T>()).

FYI Just to double-check... you are okay losing this TODO? We'll figure it out once we need it, without any TODO reminder?

---

bindings/pydrake/systems/framework_py_semantics.cc, line 273 at r1 (raw file):

           py_reference_internal, py::arg("index") = 0);
  };
  type_visit(bind_common_scalar_types, pysystems::CommonScalarPack{});

It seems like we should have _check_instantiations tests for all of the above?

---

bindings/pydrake/systems/framework_py_systems.cc, line 176 at r1 (raw file):

      // WARNING: Mutating `output` will not work when T is AutoDiffXd,
      // Expression, etc.
      // @see https://github.com/pybind/pybind11/pull/1152#issuecomment-340091423  // NOLINT

FYI Since its not doxygen you don't need @see, so it doesn't have to be on this line, and then just a URL on a line by itself can never trigger a line-length complaint (// SomeUrl is auto-whitelisted).

---

bindings/pydrake/systems/framework_py_systems.cc, line 289 at r1 (raw file):

           return self.ToSymbolic();
        })
        .def("ToSymbolicMaybe", &System<T>::ToSymbolicMaybe);

I think ToAutoDiffXdMaybe and ToSymbolicMaybe are not tested?

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 10 of 22 files reviewed at latest revision, 19 unresolved discussions.

---

bindings/pydrake/autodiffutils_py.cc, line 28 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The three new lines in this file lack direct unit test coverage?

Done.

---

bindings/pydrake/systems/framework_py_semantics.cc, line 52 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I can't tease out why this moved, especially when BasicVectorPtrList didn't move. The prior location down below seems to be to be closer to its first point of use, which is more clear to the reader.

Done. Moved the binding for AbstractValues (and other non-T stuff) up, since it used this definition and did not depend on T.

---

bindings/pydrake/systems/framework_py_semantics.cc, line 149 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Just to double-check... you are okay losing this TODO? We'll figure it out once we need it, without any TODO reminder?

OK Yup. I should have removed it earlier; primarily because it's not needed since we get auto-downcasting wiht AbstractValue.GetValue() in Python, and secondarily because binding templated methods, exposing parameters, is demonstrated in cpp_template_test.py.

---

bindings/pydrake/systems/framework_py_systems.cc, line 176 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Since its not doxygen you don't need @see, so it doesn't have to be on this line, and then just a URL on a line by itself can never trigger a line-length complaint (// SomeUrl is auto-whitelisted).

Done.

---

bindings/pydrake/systems/framework_py_systems.cc, line 289 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think ToAutoDiffXdMaybe and ToSymbolicMaybe are not tested?

Done.

---

bindings/pydrake/systems/framework_py_values.cc, line 27 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Its difficult for me to immediately understand what passing a py::module by-value means.

(Ditto for all the other type_visit lambda targets in other files.)

OK py::module inherits from py::object; both are effectively C++-friendly wrappers of PyObject*, inherent to the API of pybind11.

Would you like me to add some more doc to pydrake_pybind.h pointing to the pybind11 docs?

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Alpha-sort "Multiplexer" between "LinearSystem" and "Passthrough", for consistency?

Done.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Given that T is used above with special meaning (its actually a generic scalar marker), it seems misleading in the below code for T to not mean "various scalars".

I suggest just saying double literally below where needed.

Done.

---

bindings/pydrake/systems/systems_pybind.h, line 103 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Either the above two lists are supposed to be identical to default_scalars.h in which case its a brittleness defect not to have cross-linked comments in the two files instructing developers to keep them in sync, or else these lists do not need to be kept in sync in which case its a confusion defect how and why these two lists should evolve differently.

Done.

---

bindings/pydrake/systems/trajectory_optimization_py.cc, line 31 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW If I'm understanding correctly, I think the important fact here is that the functions return references instead of copies. If that's what you mean, it probably should be made more obvious.

Done.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

What about the VectorBase / Supervector / Subvector?

Done.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't understand what this is saying.

Done.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW typo

Done.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW ... or else fail-fast (when T is neither of the two that its supposed to be?).

Done.

---

Comments from Reviewable

[EricCousineau-TRI]

Addressed comments, PTAL.

---

Review status: 10 of 24 files reviewed at latest revision, 17 unresolved discussions.

---

bindings/pydrake/systems/framework_py_semantics.cc, line 273 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It seems like we should have _check_instantiations tests for all of the above?

Done.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

As a basic sanity check, it seems worth checking some fact about the resulting system, e.g. that the num_inputs == 1 and input_port[0].size == 1, or similar.

Done.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear why missing:

• ConstantValueSource
• ZeroOrderHold

Done.

---

bindings/pydrake/util/cpp_template.py, line 169 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Should there be a unit test to make any assertions about the quality of the exception messages that are coming out related to this?

Done. See cpp_template_pybind_test

---

Comments from Reviewable

[jwnimmer-tri]

bindings/pydrake/systems/framework_py_values.cc, line 27 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK py::module inherits from py::object; both are effectively C++-friendly wrappers of PyObject*, inherent to the API of pybind11.

Would you like me to add some more doc to pydrake_pybind.h pointing to the pybind11 docs?

So is the bottom line here basically py::object is like a pointer, so capturing by-reference is unnecessary / weird, because copies are both cheap and interchangeable with the original?

---

Comments from Reviewable

[jwnimmer-tri]

+@SeanCurtis-TRI for platform review per schedule, please.

---

Reviewed 13 of 14 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[SeanCurtis-TRI]

Out of curiosity, is the python API documented somewhere? I'm particularly interested in the Thing/Thing_ convention. That should be documented outside of the binding cpp file, but I don't know if we document any of the python-specific aspects of pydrake.

---

Reviewed 9 of 17 files at r1, 13 of 14 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[jwnimmer-tri]

AFAIK we have http://drake.mit.edu/doxygen_cxx/python_bindings.html as a guide for developers of the python bindings and http://drake.mit.edu/python_bindings.html as documentation for users.

[SeanCurtis-TRI]

Documenting the pattern for these templated classes would be a good thing there. It's something that isn't obviously pythonic.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[EricCousineau-TRI]

Added docs on the pattern for template stuff. PTAL.

---

Review status: 23 of 26 files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/systems/framework_py_values.cc, line 27 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

So is the bottom line here basically py::object is like a pointer, so capturing by-reference is unnecessary / weird, because copies are both cheap and interchangeable with the original?

OK Yup. Added some docs to pydrake_pybind.h, will see if I can upstream some of them (they have some docs, but they're not super cohesive on conversions).

---

tools/workspace/pybind11/repository.bzl, line 9 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Blocking on reaching our master.

Done.

---

Comments from Reviewable

[EricCousineau-TRI]

BTW While not completely pythonic, it has parallels to type hints introduce in Python3:
https://docs.python.org/3/library/typing.html
(pybind11 also uses the bracket syntax for error reporting on types, e.g. List[str])

---

Review status: 23 of 26 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 1 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[SeanCurtis-TRI]

Great documentation. Just a couple of small editorial quibbles.

---

Reviewed 1 of 1 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

doc/python_bindings.rst, line 185 at r5 (raw file):

binding convention is used.

For example, ``Adder<T>`` is a Systems primitive which has user-defined number

BTW "...has a user-defined number..."

---

doc/python_bindings.rst, line 186 at r5 (raw file):

For example, ``Adder<T>`` is a Systems primitive which has user-defined number
of inputs, and outputs a single port which is the summation of all of the

BTW s/summation/sum (sum is the value, summation is the process)

---

doc/python_bindings.rst, line 193 at r5 (raw file):
BTW I'm not 100% sure on the beginning of this sentence. "To access..., to then access..." It seems that the second clause supplants the first. So, simply:

To access other instantiations, you should add...

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 25 of 26 files reviewed at latest revision, 3 unresolved discussions.

---

doc/python_bindings.rst, line 185 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW "...has a user-defined number..."

Done.

---

doc/python_bindings.rst, line 186 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW s/summation/sum (sum is the value, summation is the process)

Done.

---

doc/python_bindings.rst, line 193 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I'm not 100% sure on the beginning of this sentence. "To access..., to then access..." It seems that the second clause supplants the first. So, simply:

To access other instantiations, you should add...

Done. Thanks!

---

Comments from Reviewable

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable