Add initial pydrake bindings for maliput

[jadecastro]

Add just enough to get up and running with an IDM/MOBIL system.

/cc @maddog-tri

---

This change is 

[jadecastro]

+@EricCousineau-TRI for feature review, please.

---

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

---

Comments from Reviewable

[EricCousineau-TRI]

, pending discussion items.

---

Reviewed 5 of 5 files at r1.
Review status: 4 of 5 files reviewed at latest revision, 13 unresolved discussions.

---

bindings/pydrake/maliput/init.py, line 1 at r1 (raw file):

# Blank Python module.

BTW @sherm1 We have other modules that are inconsistent with lcmtypes/__init__.py (more descriptive info).
Will chase this PR with a follow-up to make them consistent.

---

bindings/pydrake/maliput/api_py.cc, line 21 at r1 (raw file):

  py::class_<GeoPosition> (m, "GeoPosition")
      .def(py::init<double, double, double>())

BTW Can you add py::arg for each of these arguments?
An example is the bindings for Eigen::Quaternion:
https://github.com/RobotLocomotion/drake/blob/master/bindings/pydrake/util/eigen_geometry_py.cc#L171

---

bindings/pydrake/maliput/BUILD.bazel, line 42 at r1 (raw file):

    name = "api_py",
    cc_deps = [
        "//automotive/maliput/api",

Same as below.

---

bindings/pydrake/maliput/BUILD.bazel, line 55 at r1 (raw file):

    name = "dragway_py",
    cc_deps = [
        "//automotive/maliput/dragway",

cc_deps should only include libraries which are robust against ODR-violations (and not already in drake_shared_library).
(See documentation for drake_pybind_library)

---

bindings/pydrake/maliput/dragway_py.cc, line 20 at r1 (raw file):

  py::class_<dragway::RoadGeometry>(m, "RoadGeometry")
      .def(py::init<api::RoadGeometryId, int, double, double, double, double,

BTW Same as above - can you name these arguments?

---

bindings/pydrake/maliput/dragway_py.cc, line 36 at r1 (raw file):

           py::keep_alive<0, 1>());

  // SHould this be in the api?

BTW Can this be changed to a TODO?

---

bindings/pydrake/maliput/dragway_py.cc, line 40 at r1 (raw file):

      .def("ToLanePosition", &dragway::Lane::ToLanePosition)
      .def("ToGeoPosition", &dragway::Lane::ToGeoPosition);
;

Extra ;

---

bindings/pydrake/maliput/test/maliput_test.py, line 1 at r1 (raw file):

#!/usr/bin/env python

BTW Shebang not necessary.

---

bindings/pydrake/maliput/test/maliput_test.py, line 23 at r1 (raw file):

        srh = [4., 5., 6.]
        lane_pos = LanePosition(srh[0], srh[1], srh[2])
        self.assertTrue(len(lane_pos.srh()) == 3)

BTW When you add the names, could you test that constructor flavor?

---

bindings/pydrake/maliput/test/maliput_test.py, line 29 at r1 (raw file):

        geo_pos = GeoPosition(xyz[0], xyz[1], xyz[2])
        self.assertTrue(len(geo_pos.xyz()) == 3)
        for i in [0, 1, 2]:

Rather than this, you can use self.assertTrue(np.allclose(geo_pos.xyz(), xyz))

---

bindings/pydrake/maliput/test/maliput_test.py, line 53 at r1 (raw file):

        geo_pos_result = lane_0.ToGeoPosition(lane_pos)
        geo_pos_expected = GeoPosition(0., -kLaneWidth / 2., 0.)
        for i in [0, 1, 2]:

BTW Same as above, using np.allclose

---

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

            lane_1.ToLanePosition(geo_pos, nearest_pos, distance)
        lane_pos_expected = LanePosition(1., 0., 3.)
        for i in [0, 1, 2]:

BTW Same as above, using np.allclose

---

bindings/pydrake/maliput/test/maliput_test.py, line 67 at r1 (raw file):

                            lane_pos_expected.srh()[i])
            self.assertTrue(nearest_pos.xyz()[i] == geo_pos.xyz()[i])
            self.assertTrue(distance == 0.)

BTW This shouldn't be in the loop?

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

Comments from Reviewable

[jadecastro]

Review status: 1 of 6 files reviewed at latest revision, 11 unresolved discussions.

---

bindings/pydrake/maliput/api_py.cc, line 21 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Can you add py::arg for each of these arguments?
An example is the bindings for Eigen::Quaternion:
https://github.com/RobotLocomotion/drake/blob/master/bindings/pydrake/util/eigen_geometry_py.cc#L171

Done.

---

bindings/pydrake/maliput/BUILD.bazel, line 42 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

Same as below.

Done.

---

bindings/pydrake/maliput/BUILD.bazel, line 55 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

cc_deps should only include libraries which are robust against ODR-violations (and not already in drake_shared_library).
(See documentation for drake_pybind_library)

Done.

---

bindings/pydrake/maliput/dragway_py.cc, line 20 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Same as above - can you name these arguments?

Done.

---

bindings/pydrake/maliput/dragway_py.cc, line 36 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Can this be changed to a TODO?

Done.

---

bindings/pydrake/maliput/dragway_py.cc, line 40 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

Extra ;

Done.

---

bindings/pydrake/maliput/test/maliput_test.py, line 1 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Shebang not necessary.

Done.

---

bindings/pydrake/maliput/test/maliput_test.py, line 23 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

BTW When you add the names, could you test that constructor flavor?

Done.

---

bindings/pydrake/maliput/test/maliput_test.py, line 29 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

Rather than this, you can use self.assertTrue(np.allclose(geo_pos.xyz(), xyz))

Done.

---

bindings/pydrake/maliput/test/maliput_test.py, line 53 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Same as above, using np.allclose

Done.

---

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

Previously, EricCousineau-TRI wrote…

BTW Same as above, using np.allclose

Done.

---

bindings/pydrake/maliput/test/maliput_test.py, line 67 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

BTW This shouldn't be in the loop?

Done.

---

Comments from Reviewable

[jadecastro]

+@sherm1 platform review, please.

---

Review status: 1 of 6 files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[sherm1]

Apologies, but I don't think I can adequately platform review this because I am too unfamiliar with Python wrapping and with our Python conventions in general. @EricCousineau-TRI, your review looked like a solid platform review to me. Do you think it was also an adequate feature review? If not, maybe this needs a feature review from @jadecastro's team. -@sherm1

---

Reviewed 1 of 5 files at r1, 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[jadecastro]

No problem, @sherm1, and I agree about @EricCousineau-TRI for platform.
+@liangfok for feature review, please.

---

Review status: 5 of 6 files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[maddog-tri]

bindings/pydrake/maliput/dragway_py.cc, line 30 at r4 (raw file):

  // TODO(jadecastro) Revisit whether or not the following belong instead as api
  // bindings.

FYI: They should be API bindings. In fact, it is kind of odd that dragway exposes its RoadGeometry constructor. dragway should have some kind of CreateDragway(...) static function that just returns a unique_ptr<api::RoadGeometry> and keeps all the implementation details stashed under the covers as implementation details.

---

Comments from Reviewable

[jadecastro]

-@liangfok
+@maddog-tri for feature review (since you've already started)

---

Review status: 2 of 6 files reviewed at latest revision, 8 unresolved discussions.

---

bindings/pydrake/maliput/dragway_py.cc, line 30 at r4 (raw file):

Previously, maddog-tri (Matt Marjanović) wrote…

FYI: They should be API bindings. In fact, it is kind of odd that dragway exposes its RoadGeometry constructor. dragway should have some kind of CreateDragway(...) static function that just returns a unique_ptr<api::RoadGeometry> and keeps all the implementation details stashed under the covers as implementation details.

Nice. Yeah, the dragway gloop should definitely not be user-facing. Fixed.

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

bindings/pydrake/maliput/api_py.cc, line 36 at r5 (raw file):

      .def("junction", &RoadGeometry::junction, py_reference_internal,
           // Keep alive, reference: `return` keeps `self` alive.
           py::keep_alive<0, 1>());

py::keep_alive<0, 1> is redundant when py_reference_internal is already used.

Can you just use py_reference_internal? (no need to specify a comment in this case.)

---

Comments from Reviewable

[jadecastro]

Review status: 5 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/maliput/api_py.cc, line 36 at r5 (raw file):

Previously, EricCousineau-TRI wrote…

py::keep_alive<0, 1> is redundant when py_reference_internal is already used.

Can you just use py_reference_internal? (no need to specify a comment in this case.)

Done.

---

Comments from Reviewable

[maddog-tri]

Review status: 5 of 6 files reviewed at latest revision, 4 unresolved discussions.

---

bindings/pydrake/maliput/api_py.cc, line 26 at r6 (raw file):

      .def(py::init<double, double, double>(), py::arg("x"), py::arg("y"),
           py::arg("z"))
      .def("xyz", &GeoPosition::xyz);

For this accessor (and the similar, LanePosition::srh()), as I (am starting to) understand pydrake/pybind11, these will return numpy-wrapped copies of the original Eigen vectors.

The C++ accessors return const-references (i.e., view semantics) --- so perhaps it would be better if the pydrake wrappers did the same? I think adding py_reference_internal will accomplish that.

Opinions, @EricCousineau-TRI ?

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

bindings/pydrake/maliput/api_py.cc, line 26 at r6 (raw file):

Previously, maddog-tri (Matt Marjanović) wrote…

For this accessor (and the similar, LanePosition::srh()), as I (am starting to) understand pydrake/pybind11, these will return numpy-wrapped copies of the original Eigen vectors.

The C++ accessors return const-references (i.e., view semantics) --- so perhaps it would be better if the pydrake wrappers did the same? I think adding py_reference_internal will accomplish that.

Opinions, @EricCousineau-TRI ?

Yes, I believe py_reference_internal may work, though you may have to wrap it to return something like Eigen::Ref<>.

I'd say it doesn't matter if there's no explicit use case for referencing, and would be relatively easy to change if needed.

One additional word of caution is that I need to resolve (with Kitware's help) whether we can maintain reference semantics with AutoDiff / Symbolic:
#8116
If we have to go with (b), then we'd have to change it.

---

Comments from Reviewable

[jadecastro]

Review status: 4 of 6 files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/maliput/api_py.cc, line 26 at r6 (raw file):

Previously, EricCousineau-TRI wrote…

Yes, I believe py_reference_internal may work, though you may have to wrap it to return something like Eigen::Ref<>.

I'd say it doesn't matter if there's no explicit use case for referencing, and would be relatively easy to change if needed.

One additional word of caution is that I need to resolve (with Kitware's help) whether we can maintain reference semantics with AutoDiff / Symbolic:
#8116
If we have to go with (b), then we'd have to change it.

Done. py_reference_internal added.

---

Comments from Reviewable

[maddog-tri]

Pass complete; PTAL.

---

Reviewed 1 of 5 files at r1, 1 of 5 files at r3, 1 of 2 files at r4, 2 of 3 files at r5, 1 of 1 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

bindings/pydrake/maliput/api_py.cc, line 18 at r6 (raw file):

  using namespace drake::maliput::api;

  m.doc() = "Bindings for the Maliput API.";

Probably a good idea to add a TODO indicating that these bindings are a work-in-progress, and referencing #7918 as tracking the progress (kind of).

---

bindings/pydrake/maliput/api_py.cc, line 21 at r6 (raw file):

  py::class_<RoadGeometryId>(m, "RoadGeometryId")
      .def(py::init<std::string>());

The string() accessor should also be exposed; that's kinda the absolute minimum to do anything useful with RoadGeometryId. (And, then, exercise it in the unit-tests, too.)

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

bindings/pydrake/maliput/api_py.cc, line 26 at r6 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

Done. py_reference_internal added.

BTW You may need to ensure this returns Ref<>. At present it's untested? (if there's little motivation to test this, then it should be fine if it just returns a copy?)

---

Comments from Reviewable

[EricCousineau-TRI]

bindings/pydrake/maliput/api_py.cc, line 26 at r6 (raw file):

Previously, EricCousineau-TRI wrote…

BTW You may need to ensure this returns Ref<>. At present it's untested? (if there's little motivation to test this, then it should be fine if it just returns a copy?)

BTW After checking the code, I was wrong, you shouldn't actually need Ref<> - sorry about that!

---

Comments from Reviewable

[jadecastro]

Review status: 4 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/maliput/api_py.cc, line 18 at r6 (raw file):

Previously, maddog-tri (Matt Marjanović) wrote…

Probably a good idea to add a TODO indicating that these bindings are a work-in-progress, and referencing #7918 as tracking the progress (kind of).

Done.

---

bindings/pydrake/maliput/api_py.cc, line 21 at r6 (raw file):

Previously, maddog-tri (Matt Marjanović) wrote…

The string() accessor should also be exposed; that's kinda the absolute minimum to do anything useful with RoadGeometryId. (And, then, exercise it in the unit-tests, too.)

Done. (unit test added also)

---

Comments from Reviewable

[maddog-tri]

---

Reviewed 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable