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

framework_py: Add test of using AbstractValue's in input ports, fix overload error for unique_ptr in pybind #8094

Merged

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Feb 17, 2018

Closes #8074

Address part of AbstractValue point in #7660. (Just need to follow-up with unrestricted updates.)

When I was first prototyping accepting unique_ptr as arguments in pybind, I introduced a bug where any registered pybind type would match with any other pybind type. This is fixed via RobotLocomotion/pybind11#10
(I ultimately made this a possibility when supporting anti-slicing stuff for shared_ptr, but just hadn't incorporated it until now...)

This also fixes a minor issue with cpp_template - any instantiations may have had the wrong module name used because inspect was not called with the right frame count. That has now been fixed.


This change is Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

Fixed the random errors in the pybind PR (due to recycled memory, instance registration stuff).

+@jadecastro for feature review, please.
+@sammy-tri for platform review, please.


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


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

Pybind PR has landed - updated in pybind/repository.bzl.

@jadecastro
Copy link
Contributor

Pass complete.


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


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

        [](const System<T>* self, const Context<T>& arg1, int arg2) {
          return self->EvalVectorInput(arg1, arg2);
        }, py_reference_internal)

BTW, comparing with the abstract binding, it no longer makes sense to me for this one to be a py_reference_internal, but instead a py_reference, py::keep_alive<0, 2>() like below since it's the lifespan of the context we want to control?


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

  py::class_<PassThrough<T>, LeafSystem<T>>(m, "PassThrough")
    .def(py::init<int>())

BTW, could you also check/add a test for this (vector) version of the ctor?


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

    def test_abstract_pass_through(self):
        model_value = AbstractValue.Make("Hello world")
        input_value = model_value.Clone()

BTW, I don't think this needs to be cloned as a separate value.


Comments from Reviewable

@sammy-tri
Copy link
Contributor

:lgtm: (pending existing issues/cleanup)


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


Comments from Reviewable

@EricCousineau-TRI EricCousineau-TRI force-pushed the issue/8074 branch 4 times, most recently from f5b2129 to 6c406cd Compare February 20, 2018 15:56
@EricCousineau-TRI
Copy link
Contributor Author

Review status: 2 of 7 files reviewed at latest revision, 3 unresolved discussions.


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

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, comparing with the abstract binding, it no longer makes sense to me for this one to be a py_reference_internal, but instead a py_reference, py::keep_alive<0, 2>() like below since it's the lifespan of the context we want to control?

Done. Good catch - thanks!


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

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, could you also check/add a test for this (vector) version of the ctor?

Done.


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

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, I don't think this needs to be cloned as a separate value.

Done.


Comments from Reviewable

@jadecastro
Copy link
Contributor

:lgtm:


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


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Feb 20, 2018

Deferring merge until #8110 lands.

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.

pydrake: AddValueInstantiation doesn't place the class in the correct module
3 participants