Drake: Context::FixInputPort does not error when the input port has been previously connected to a valid source

[rcory]

I found this issue using pydrake. Take the following pydrake code as an example:

# Add the MBP.
plant = builder.AddSystem(MultibodyPlant())

# Add a constant actuation source.
const_src = builder.AddSystem(ConstantVectorSource([1]))
builder.Connect(const_src.get_output_port(0), plant.get_actuation_input_port())

# Build the diagram and get the context.
diagram = builder.Build()
diagram_context = diagram.CreateDefaultContext()
plant_context = diagram.GetMutableSubsystemContext(plant, diagram_context)

# Now fix the same input port that was just connected.
plant_context.FixInputPort(plant.get_actuation_input_port().get_index, [0])

This code runs without warning, where FixInputPort shadows the previous call to Connect. I would have expected an error indicating that an input port that has already been connected is now being declared as fixed.

[jwnimmer-tri]

Can you add more detail (code) for the elided "Get the plant context" part, please? My intuition is that this is working as intended, but its tough to tell without knowing where plant_context came from.

[rcory]

@jwnimmer-tri I've updated the example. For reference, you can find a concrete pydrake example (modified cart pole) in this branch, which happily runs with no warnings/errors.

[sherm1]

Per f2f the issue is that @rcory unintentionally provided a fixed value for an internal input port that had already been connected to a source subsystem's output port. The result was that the system ran happily, but ignored the source subsystem altogether. Although that is functioning as built, Rick would have preferred to get an error message. Ideally the FixInputPort() call would have failed saying the input port already had a value. An alternative would be to catch it later when first evaluated -- that might be easier to implement since both system and context are available, but doesn't seem as helpful.

The other question to resolve is whether we _should_ allow FixInputPort() to override the value for an already-connected input port. At the moment I don't see that as being useful enough to justify the bad UX described above.

[tri-ltyyu]

Still in discussion on whether error should be produced

[jwnimmer-tri]

As currently written (and very carefully so -- see ContextBase::SetFixedInputPortValue), a user can call FixInputPort twice for a given port on the same Context, without error. I don't see a good reason why changing an input port value that's been bound via Connect should be treated differently that an input value bound via Fix. In either case, we are letting the user supply new runtime value for a port, overriding any previous value. It does what it says on the tin. If we want to fail-hard on Fix-after-Connect, I think we should be consistent and fail-hard on Fix-after-Fix. Which is to say, I don't think that this should be an error.

[sherm1]

FWIW I think fix-after-fix working is expected behavior (just one value source for the port, with the value updated explicitly) while the fix-an-already-connected port (two independent values, with one quietly overriding) may be bad UX. The symptom Rick reported is that his System quietly ran with the accidental fixed port, diligently producing bad answers.

I think given that we have an actual bad user experience reported here, and no use case I'm aware of where overriding an already-connected port is actually needed, we should treat this as an error. It would be possible to add an an explicit FixAlreadyConnectedPort() API later if we needed one.

[rcory]

I agree with @sherm1 - seems like bad UX from my point of view.

If we do decide to keep the behavior as is, it should probably be made clearer in the docs. I noticed that the documentation for Context::FixInputPort (the function that is called in my example) makes no mention of the "override" behavior that occurs when fixing an already connected port.

As a matter of fact, looking one level further, ContextBase::FixInputPort says "This is the most general way to provide a value (type-erased) for an unconnected input port", but makes no mention of how already connected ports are affected.

[jwnimmer-tri]

I think if one imagines "Fix" as "Connect the input to a context-sourced constant and then Set the initial value for the source to be foo", then it's valid to think of it as an error when "the port was already Connected to something else", because calling Connect twice is an error during diagram building. _Edited to add: And with this mindset, calling Fix twice is an error, since it's also two Connect calls._

However, if one images "Fix" as "Set the value of this input to be x", then setting the value more than once is not an error, and indeed if the port already had a value then Set should replace the old value, even if the value came from a prior Connect operation.

Maybe the right answer is to tease apart those operations, or rename the methods, or offer more than one method, etc.

I don't want to (continue to) erode the ability of the user to mutate a System-Context and then re-simulate or re-compute after the modification. We should allow users to disconnect and reconnect ports, unfinalize and add or remove systems, etc. They should not need to throw away the Diagram and start over every time they want to make a change. That leads us deep into the land of red-functions vs blue-functions (aka Finalize is evil). We should err on the side of allowing modifications to the Context, even after a Diagram is Finalized.

I completely agree that we should work to alleviate user confusion and give good diagnostics on mistakes, but we also should not artificially lock down our capabilities in doing so.

[sherm1]

I don't think there is a deep philosophical principle involved here. We don't have a use case for making an internal, already-connected input port ignore the output port it's connected to. We do have a user (from our targeted user community) who did that by accident and was surprised and inconvenienced by the quietly-wrong answers he got, and thought he should have gotten an error message. With just this data, why wouldn't we want to make the user's life easier?

I'd like to hear from other Drake users on this. Here's the question:

• You have a Diagram with internal subsystems that have their ports connected.
• If you apply FixInputPort(value) to one of the _internal_ input ports, which behavior would you prefer:
  :+1: the input port subsequently ignores its connected output port and always reads value
  :-1: you get an error message saying "this port is already connected".

The argument for :+1: is that it gives you more ability to modify the Diagram's behavior without having to rebuild it (is there a use case?). The argument for :-1: is that this is likely to be a user mistake (as it was for @rcory here) and the error message would avoid that.

Please vote on this comment with :+1: for allowing the input port override, :-1: for the error message and add your own comments if you have some additional thoughts as to which way we should go. Thanks!

[jwnimmer-tri]

I will make a use case if you want one. They are not difficult to think of. But in general, we really, really should not limit what computations our users can formulate. If you want to offer it under a different API so that you get your error message that's fine, but I dispute that we should seal off the ports on purpose, forever.

[mitiguy]

From a different perspective .....
Perhaps it is worthwhile to consider a "MessageManager" class
that outputs a log (or file) with useful messages.

1. Error: User behavior which are definite errors.
   This text file would be helpful for a frustrated end-user to forward to a developer.
2. Warning: Something seems "strange", such as Rick's example.
   Quoting Rick "The silence was disturbing".
   A message log with warnings would be helpful.
3. Notes: Possible log of data that is useful for debugging.

[jwnimmer-tri]

Indeed, that was another tack I was considering suggesting. If a user can't understand why their simulation is computing stupid answers, that needs to have a general solution (beyond warning about suspicious connections). Tools like "watch the diagram of the running simulation, with input/output/state values changing over time, and connections shown with boxes and arrows". That sounds like something worth our time.

[rcory]

Tools like "watch the diagram of the running simulation, with input/output/state values changing over time, and connections shown with boxes and arrows". That sounds like something worth our time.

That would be fantastic. One tool which I used to leverage heavily in Simulink was the scope, which easily connects to any part of the diagram and instantly visualizes the signal. We'd of course want the "drake scope" to be able to interpret any type signal (e.g., AbstractValues) in some automatic way.

Having said that, I also agree with @mitiguy that having some sort of messaging/logging system would be helpful. Better introspection tools will help (and might eventually be the only tools we need when available), but producing something other than silence in situations like this would be much more helpful right now, in my opinion.

Perhaps just building in debug should trigger a meaningful warning.

[avalenzu]

I voted :+1: above, but with the caveat that a warning should be emitted (at least in debug mode).

[SeanCurtis-TRI]

Without too much deep thought, I'd favor an error, but with an additional API for disconnecting. That way making connection is not an irreversible act, but silently stomping on connections would be prevented.

[sherm1]

I don't think we have sufficient consensus yet for making a change. @rcory what do you think about us addressing this for now just by updating the documentation to properly explain the current behavior? We could then wait to see whether the same bad-UX occurs again -- maybe this was just an unlucky accident unlikely to be repeated?

[rcory]

Per f2f discussions, I'd be ok with just updating the documentation for FixInputPort as well as Connect for now. Having both document the behavior that a connection made via Connect can be overridden by FixInputPort. This will at least avoid some of the confusion.

@jwnimmer-tri, not to belabor the discussion, but I'm wondering what use case you have in mind where prohibiting a call to FixInputPort on an already connected port would break the user's workflow? I'm having trouble thinking of some.

[jwnimmer-tri]

One example: you're doing an interactive pydrake session. You're partway through a sim, and you want to force a new value into a (nested) port and manually check a what some outputs look like now, or visualize the resulting pose, without having to figure out how to make the output port take on that value.

[sherm1]

Well, I have to say that I find that made-up use case entirely unconvincing! But it doesn't matter for now. Per f2f with Rick, he will submit a PR to fix the documentation for FixInputPort and Connect to explain the current behavior. We'll assume for now this was just bad luck and that other users won't suffer the same fate. If it happens again we'll reconsider.

FWIW, the current behavior that Rick is going to document is essentially accidental. I rewrote the FixInputPort implementation last year as part of the caching project and don't remember thinking about this side effect at all. I _believe_ the previous implementation, which was much more complicated, required input ports to be _either_ connected or fixed.

[jwnimmer-tri]

I think the appropriate debate to have is what we want it to be in the future, not what it happened to be in the past. Context is a value-like object. It has getters and setters. To the extent we curtail its getters and setters to operate in fewer-than-all use cases (i.e., that certain setters refuse to operate when the moon is just right, because it was "probably not what you wanted"), and there is _no way_ to ever work around that limitation, it makes the toolbox more difficult to apply to cases that MultibodyPlant didn't happen to need. The Context should do the simple, obvious thing, without preconditions, and without the user needing to consult a lot of documentation. I really do think that Connect vs Set is an important and fundamental confusion in this issue, and that spending a little time clarifying those two distinct operations on the Context would solve all of these problems. (I also think that making a scope is way better that writing complicated documentation that nobody will read.)

[sherm1]

I don't think we're communicating successfully Jeremy; we would probably have to discuss this f2f. However, unless you are _objecting_ to Rick documenting the current behavior, I will assume we don't need to sort this out now and that Rick's PR will close this issue.

[rcory]

I'll go ahead and put together a PR that updates the documentation, unless anyone opposes.

Perhaps we can plan to pick this discussion up at the on-site, which might also be a good opportunity to have a more general discussion on what tools we'd like to provide for user development.