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

py systems: Bind OutputPort.Allocate(), DiagramBuilder.empty(), .GetMutableSystems() #13270

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented May 10, 2020

Motivation is to be able to make an "auto" printer system.

Using the FunctionSystem prototype I mentioned in Slack, I can do something like this:

def make_printer(cls, fmt=None, print=print):
    if fmt is None:
        fmt = "{}"

    def print_value(value: cls) -> None:
        print(fmt.format(value))

    printer = FunctionSystem(print_value, set_name=False)
    return printer


def connect_printer(builder, output, dt, fmt=None):
    cls = type(output.Allocate().get_value())
    printer = make_printer(cls, fmt=fmt)
    builder.AddSystem(printer)
    builder.Connect(output, printer.get_input_port(0))

...
# No redundant info necessary.
connect_printer(builder, system.get_output_port(0), 0.1)

This change is Reviewable

@EricCousineau-TRI EricCousineau-TRI added the status: single reviewer ok https://drake.mit.edu/reviewable.html label May 10, 2020
Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@sherm1 for both reviews, please.
+(status: single reviewer ok)

Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

@EricCousineau-TRI EricCousineau-TRI changed the title py systems: Bind OutputPort.Allocate() py systems: Bind OutputPort.Allocate(), DiagramBuilder.empty(), .GetMutableSystems() May 10, 2020
@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented May 10, 2020

Added .GetMutableSystems() because I would like to get an added system by name (rather than explicitly renaming it) during building time. This is the easiest route to do so in Python.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: platform and feature (sort of -- looks reasonable but my pybind expertise is insufficient to say whether the py::casts and keep_alive are the right incantations). I'm satisfied if you are though.

Reviewed 2 of 2 files at r1.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

@EricCousineau-TRI EricCousineau-TRI added the status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits label May 10, 2020
@EricCousineau-TRI
Copy link
Contributor Author

Thanks! The per-element py_keep_alive is a bit conservative - I could have just done keep-alive on the returned list; however, if a user were to mutate that list for whatever reason, it may screw up the keep-alive-ness (e.g. the user deletes a system from the list, the keep alive no longer applies to that system, and could cause a read-after-delete segfault if the Python GC acts too early on the builder).

@EricCousineau-TRI EricCousineau-TRI merged commit acb0e04 into RobotLocomotion:master May 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants