proposal: `OutputPort::get_leaf_system()`

[RussTedrake]

Moving the discussion in PR #9911 to it's own issue.

The problem: 1) consumers of the diagram ManipulationStation (and others that are like it) have to dig into the internals of the diagram in order to successfully wire up the external visualization routines to the output port. ConnectDrakeVisualizer() requires a pointer to the SceneGraph and ConnectContactResultsToDrakeVisualizer requires a pointer to the MultibodyPlant. 2) Those original API for those methods (ConnectDrakeVisualizer and ConnectContactResultsToDrakeVisualizer) took only the scene_graph/mbp as an argument, and acquired the OutputPort& from the system pointer. But this doesn't work if those systems are inside a diagram. So I've had to offer a second API entry point where the ports are passed independently, with documentation saying they have to be connected somehow (ConnectDrakeVisualizer and ConnectContactResultsToDrakeVisualizer).

@jwnimmer-tri expressed the view that the signal on the port should perhaps have enough information contained in it that everything required for (in this case) visualization should be available to the publishers by simply evaluating the port. I can see the appeal, but this feels like an extremely heavy burden to put on every signal... to publish (lots of) data that is constant over a simulation on the port.

My proposed solution that I like: OutputPort already has a method

  /** Returns a reference to the System that owns this output port. Note that
  for a diagram output port this will be the diagram, not the leaf system whose
  output port was forwarded. */
  const System<T>& get_system() const ...

If we additionally offered e.g.

  /** Returns a reference to the LeafSystem that is the original source for this 
  output port. For a leaf system output port, returns this is equivalent to `get_system()`. 
  For a diagram output port, returns the LeafSystem whose output port has been
  exported by the diagram or nested diagrams. */
  const System<T>& get_leaf_system() const ...

In this particular use case, it means that the Connect methods could have a single entry point that takes the OutputPort& only, and extract the leaf system pointers from that reference. I think that is conceptually a cleaner pattern for wiring anyhow than passing in the system reference.

cc @jwnimmer-tri , @sherm1 .

[sherm1]

I see two independent issues here:

1. Should a diagram output port be able to reveal the actual source of the port's signal?
2. If so, should this feature be used to improve these particular APIs?

The first question is about introspection -- a special case of "should the System framework support introspection?" I am enthusiastically in favor. However, I would prefer that the API be

virtual const LeafOutputPort<T>& get_source_leaf_output_port() const;

from which the LeafSystem may be extracted. The reason I prefer that is that it is minimal and sufficient - it reveals the actual source of the Diagram output signal so any further introspection can be done without more additions to the OutputPort API.

BTW DiagramOutputPort already has a method (unused, I think) called get_source_output_port() but that refers to the immediate source, which may itself be a Diagram.

I don't have a strong opinion about the APIs for the diagram-building helper functions as long as they are useful and well-documented. The System framework establishes an elegant model for how Systems should operate, but I don't think we need to be nearly as strict while a System is being built.

[jwnimmer-tri]

As always, I remain sternly against breaking the Diagram abstraction barrier, using methods other than those on Diagram itself. It destroys the part-whole architecture.

[sherm1]

@jwnimmer-tri, do you feel that just being able to ask an OutputPort for its source OutputPort breaks the abstraction? Or are you worried about how it might be used?

[jwnimmer-tri]

... breaks the abstraction?

Yes. IMO, if you have a System* pointer, you should only ever be able to treat it as a single block -- as if it were a LeafSystem, or a VectorSystem, or etc. If you want to peek inside the block, you should be required to cast it to Diagram.

Or are you worried about how it might be used?

Yes, also this.

[RussTedrake]

I agree with the sentiment. But also think we must find a way for it to be ok to have a MultibodyPlant or SceneGraph inside a diagram. I think the manipulation station is a very clear example of where there is value in having a System block representing a collection of subsystems, which include MBP/SceneGraph.

A solution to the immediate need that might be made to work could be:

• only provide GetSourceOutputPort for DiagramOutputPort
  • ConnectDrakeVisualizer only takes an output port, calls get_system on the output port and dynamic casts to SceneGraph
  • We offer an overload of ConnectDrakeVisualizer that takes a DiagramOutputPort, which uses get_source_output_port().get_system().

Unfortunately, users of the Diagram can't/don't normally get a DiagramOutputPort& since get_output_port, etc return the OutputPort&. Also, to get the nested diagram case, the Diagram class (or DiagramOutputPort) would have to presumably overload/specialize AddSystem for derivatives of Diagram and keep an internal record of which systems are down-castable to 'Diagram` for the recursion.

cc @EricCousineau-TRI (who chimed in on #10009)

[jwnimmer-tri]

If the immediate need relates to passing the SceneGraph reference into ConnectDrakeVisualizer, then I think it's weird to be adjusting the framework for this reason. The only use of SceneGraph within ConnectDrakeVisualizer is to call DispatchLoadMessage, and there's already a TODO right there that says its wrong to be copying data out of the SceneGraph member fields for this purpose. The use of the System reference during ConnectDrakeVisualizer should be removed entirely, not obscured.

To me, the problem in the top post sounds like one of ergonomics, not of a fundamental inability to, e.g., formulate some computation. If that's the case, then I'd much rather fix the broken concrete system implementations, than add the "get_leaf_output_port" concept to the base output port.

[RussTedrake]

@jwnimmer-tri -- The same pattern happens in the ConnectContactResultsToDrakeVisualizer system (and it was your review comments there that started this discussion).

It's a good question as to whether this will happen elsewhere. The other pattern that we have been using (which we also consider broken, #9930) is when systems need to consume, e.g. the state vector from a MBP and work with it. Here they also need to know more about the MBP than is available through the port, and currently get it by taking a const reference to that MBP in the constructor. Most of these systems have size checks on the inputs, but I believe that is the only check to make sure that the the signal data matches the MBP passed in the constructor.

[jwnimmer-tri]

... ConnectContactResultsToDrakeVisualizer ...

Right. The contact results output port should offer sufficient information to convey its meaning. I don't understand the objections to that (or maybe there aren't any)? It's certainly not a runtime performance question -- providing data or APIs on a port's value type doesn't imply deep copies of the data.

... other pattern ... systems need to consume, e.g. the state vector from a MBP ... they also need to know more about the MBP than is available through the port, ...

If you can link to one or two illustrative examples of this to consider, then we account for them in this discussion as well.

[RussTedrake]

https://github.com/RobotLocomotion/drake/blob/master/systems/controllers/test/inverse_dynamics_controller_test.cc#L118

But it happens everywhere. Any time we take a System (not just an MBP/SceneGraph) and make a state estimator, a controller, perform trajectory optimization, etc.. we are producing a new system/trajectory that produces signals with the coordinate system that was only defined by the original System. If it was wired up to the wrong system, then at best we would be catching it via size mismatch.

My argument against adding all information to every port is not about runtime performance. It is about being able to think (and write algorithms against) a parsimonious (minimal?) description of the signal. I don't know how to take gradients of System pointers, etc. I'm not saying that there is no good solution along this path, but we've gotten pretty far with the current paradigm.

And one could potentially argue that adding the proposed method to the OutputPort is precisely a mechanism to adorn the port (but not it's runtime transmissions) with the information required.

[sherm1]

IMO, if you have a System* pointer, you should only ever be able to treat it as a single block -- as if it were a LeafSystem, or a VectorSystem, or etc. If you want to peek inside the block, you should be required to cast it to Diagram.

I'm not sure what we gain by that, especially during Diagram building. If I'm given a System with its OutputPort, it's not a secret that the System might have subsystems or that the OutputPort might have been exported from one of them. Getting the source OutputPort is something any System can provide without the caller needing to know whether the System has subsystems. Not sure why that's a bad thing.

OTOH, I will confess to (conscious) bias on my part since as you know I have always felt the System concept alone was sufficient, where a System can contain zero or more sub-Systems. I have yet to understand what we gained by elevating the Diagram/LeafSystem distinction to more than that. So I'm inclined to be sympathetic to a user who is having trouble working with those abstractions.

[EricCousineau-TRI]

I've still got trepidation on encouraging this pattern; however, I could see it as a stepping stone towards putting the necessary data on the wire at some point, since the API would be about the same (with fewer preconditions, too).

Perhaps that would come in more naturally once we support adding/removing geometry?

[sherm1]

Perhaps that would come in more naturally once we support adding/removing geometry?

I'm not getting the connection Eric -- can you elaborate?

[sherm1]

We are having trouble figuring out what is the right way to deal with the user problems caused by the MBP+SG pairing. I think we need to experiment with various approaches in real user code until we converge on something that works. I propose that we add this method (merge #10009) and then review the uses of it as they come up in practice (for functionality, clarity, fragility, etc.), rather than continue to discuss it in the abstract. Objections? Counterproposals?

[jwnimmer-tri]

I still have yet to see a use case where there isn't a trivial work-around without violating part-whole. (The ID link is also trivially solved.) I don't see what the rush is.

[jwnimmer-tri]

... problems caused by the MBP+SG pairing.

To be clear, so far this issue has not been about that twaining, but rather that downstream systems that consume either an MBP port or a SG port need metadata about the values on that port, and so need to be handed the concrete System subclass reference in order to ask it for the metadata.

I think we need to experiment with various approaches in real user code ...

Having even taken 30 seconds to try to start prototyping the ConnectDrakeVisualizer change that doesn't require get_leaf_system(), I see that in any case we should never take just the pose_bundle_output_port -- we also need the geometry (shape / color / etc) information beyond the poses, which is either the query port or some to-be-determined port -- definitely not the poses port. So trampolining the pose port into the SG object won't work in any case -- taking the query port from that SG would be the SG System's port, not the Diagram query port.

So in short, yes, I'm a fan of debating this issue in code. But let's debate the places where this idea is applied rather than a framework change that putatively solves the problem, but we won't know until we evaluate the call site changes, and so far does not solve the real problem anyway.

[sherm1]

Is this issue dead now, @RussTedrake ?

[RussTedrake]

no. i think the problem is still real, and needs a solution.

[RussTedrake]

I'm willing to close this now; I haven't been pining for it and it was obviously controversial.