RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.29k stars 1.26k forks source link

Option to declare outputs as direct-feedthrough, sidestepping symbolic form #12253

Closed jwnimmer-tri closed 4 years ago

jwnimmer-tri commented 4 years ago

Currently on master, when MultibodyPlant is asked for its direct-feedthrough pairs (while finalizing a Diagram), it attempts to use symbolic form to infer the sparsity of the reaction forces output port.

Ideally we'd like to have MultibodyPlant express to the framework that that output is direct-feedthrough from all inputs, without needing to call into all of the symbolic code paths.

However, the systems framework does not offer an option for that kind of declaration. You can declare that a specific output is direct-feedthrough from only some (or no) inputs, but even if you declare that it depends on specific inputs the framework will still attempt to use symbolic form to narrow that list.

This isn't a huge deal (it's only a runtime performance impact, perhaps only during one-time diagram building), but it is surprising to see it happening.

sherm1 commented 4 years ago

It seems odd to have to defend the output ports from symbolic analysis. Maybe we should just believe the output port dependency specification and avoid any attempt at second-guessing? It does mean there would be an epidemic of false positive pass-throughs until people correct the output port dependency specs -- but only in those few systems that actually support transmogrification to symbolic.

sherm1 commented 4 years ago

FYI for those not familiar with this dark corner of the System framework, it currently works like this. LeafSystem implements GetDirectFeedthroughs() as follows:

jwnimmer-tri commented 4 years ago

Maybe we should just believe the output port dependency specification and avoid any attempt at second-guessing?

If the user supplied an output port's prerequisites_of_calc as anything other than its default value, I would be okay with skipping the symbolic analysis for that port.

If the user did not provide a prerequisites_of_calc (so would currently have prerequisites_of_calc = {all_sources_ticket()}), I think its still worth looking at symbolic form when the symbolic form is available.

RussTedrake commented 4 years ago

Update: I've hit a case where this is not only a performance issue, but challenges my ability to support symbolic::Expression in the Controller class in finite_horizon_linear_quadratic_regulator.cc.

Short version: I need to use ExtractDoubleOrThrow(context.get_time()) in the output port calc method, but want to support symbolic for everything except time. This worked fine, until I put my system in a Diagram and was unable to opt-out of the SymbolicSystemInspector trying to read my output port (and throwing).

If I'm going to implement the finite-time invariance with sums-of-squares (and the rest of the LQR-Trees algorithm) then I need symbolic support for this controller.

sherm1 commented 4 years ago

A straightforward fix would be to add an "(Advanced)" System method like suppress_symbolic_passthrough_inspection() that would set a bool somewhere. Is that too much of a hack?

jwnimmer-tri commented 4 years ago

I'd hope that "if the user supplied an output port's prerequisites_of_calc as anything other than its default value" would not be very much code to add?

sherm1 commented 4 years ago

I don't think that's in the same league of easy fixes. A few issues:

jwnimmer-tri commented 4 years ago

You're moving the goalposts.

Your proposal was a small hack that tries to accommodate the situation for now, until we find something more broadly suitable. It leaves problems unsolved and questions unanswered, but allows for some expert users (Russ) to trailblaze in the meantime.

Your objections to my proposal were that it does not solve the whole problem robustly -- that it has loose ends. You are right, but it was not my intent for it to be the final answer. We would keep this issue open and work toward a broader solution. It was intended to be an equally-easy hack that as well helps trailblazers, but has the advantage that the obvious thing that Russ already tried would have worked.

RussTedrake commented 4 years ago

I understand Jeremy's proposal as (for now), if someone manually specifies the all_sources_ticket() then they may still get symbolic inspection. But if someone cuts more specifically, then they can avoid it. I feel like that is more aligned with the long term solution, but would also be ok with the "advanced system method" backdoor.

The very simple "overload DoHasDirectFeedthrough()" we had before also worked just fine, from my perspective.

sherm1 commented 4 years ago

It was intended to be an equally-easy hack that as well helps trailblazers, but has the advantage that the obvious thing that Russ already tried would have worked.

I don't think it's an equally-easy hack, though I don't think it would be too hard if we don't distinguish "took default" from "has default value". The fact that it would have worked for Russ is a good argument in favor though!

sherm1 commented 4 years ago

I'll push a PR for Jeremy's proposal here soon (if no one beats me to it).