Untangle context from system, remove input_port_evaluator_interface

[sherm1]

This is the next modest-size, hopefully coherent PR from the caching branch #7668.

This removes the unnecessary dependency of Context on System for input port evaluation, and eliminates the need for the InputPortEvaluatorInterface. Now input ports are evaluated by the System with the Context being consulted only in the case of fixed input ports, which don't need evaluation. Lots of input port-handling code gets deleted here, and much more of it will be deleted in subsequent PRs.

Scalar type-independent System name, pathname, and parent code moves up to SystemBase and some duplicated code gets the ax. More type-independent code will move up in subsequent PRs.

(The code is moving towards more even treatment of input and output ports. Think of InputPortDescriptor as renamed to InputPort. I will do that after some more input port surgery.)

---

This change is 

[sherm1]

+@jwnimmer-tri for feature review (not a big PR)
+@EricCousineau-TRI for platform review please, per vacation schedule

---

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

---

Comments from Reviewable

[jwnimmer-tri]

FYI I have spent a few hours on this already, and still will need a few more before I finish. This PR is altering some fundamental premises about how input-output wiring / responsibilities / visibility takes place, as well as altering some fundamental decisions about how system names are handled. I should be able to finish a first pass before tomorrow morning, but just be advised on that timeline.

---

Comments from Reviewable

[sherm1]

Thanks, Jeremy. This and coming changes to inputs and outputs should be functionally neutral. And I didn't intend to change system names now either, just consolidate the code and move non-typed code into the non-typed base class. There was a lot of unnecessary complexity in the port handling that interfered with my ability to cache things. Next steps will be to move much of the type-erased port functionality up to the type-erased classes, and to further reduce Context's involvement in input port evaluation to just freestanding port values, which require no evaluation.

---

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

---

Comments from Reviewable

[jwnimmer-tri]

I have published some small comments in case you want to pipeline some fixes while I continue reading.

This and coming changes to inputs and outputs should be functionally neutral.

I agree that using-code is probably unaffected. It changes the design and specification for how Systems / Diagrams / Context interact with each other, and what abstractions are being chosen to partition their responsibilities. That is a design review, and needs some time from me to work through the reasoning.

---

Reviewed 13 of 17 files at r1.
Review status: 13 of 17 files reviewed at latest revision, all discussions resolved.

---

systems/framework/system.h, line 405 at r1 (raw file):

    if (abstract_value == nullptr) return nullptr;
    const BasicVector<T>& basic_value =
        abstract_value->GetValue<BasicVector<T>>();

GetValue means static cast. It is unclear to me how we know, as of this line, that the abstract_value definitively holds a BasicVector<T> as its erased type.

Similarly, it is unclear to me how we meet our contract "Throws std::bad_cast if the port is not vector-valued". I also do not see a unit test for that promise.

---

systems/framework/system.h, line 1352 at r1 (raw file):

  // Diagram builder ensures we have the same scalar type all the way up.
  const System<T>* get_parent() const {
    return static_cast<const System<T>*>(get_parent_base());

At least we should ASSERT that the cast is going to succeed, since the guarantor of it is not nearby (not easy to reason about).

---

systems/framework/system_base.h, line 38 at r1 (raw file):

  void set_name(const std::string& name) { name_ = name; }

  /** Returns the name last supplied to set_name(). */

Even though the comment that used to live on the System::get_name was ported to live on GetSystemName now, I think it actually still makes more sense here. The current short sentence here does not tell the whole story.

Also, the GetSystemName is an override, so adding detailed semantics to its API docs that are not part of the base interface is odd, at best.

---

systems/framework/system_base.h, line 454 at r1 (raw file):

  // Diagram. The enclosing Diagram is needed for interactions between peer
  // subsystems via input and output ports. Aborts if the parent has already
  // been set to something else.

Public methods should Doxygen style.

---

systems/framework/system_base.h, line 454 at r1 (raw file):

  // Diagram. The enclosing Diagram is needed for interactions between peer
  // subsystems via input and output ports. Aborts if the parent has already
  // been set to something else.

FYI Re-flow this comment block? The first line is very short.

---

systems/framework/system_base.cc, line 10 at r1 (raw file):

std::string SystemBase::GetSystemPathname() const {
  std::vector<const SystemBase*> path_to_root{this};
  while (const SystemBase* parent = path_to_root.back()->get_parent_base())

I originally thought that my confusion here was the lack of braces. I do think that's still a readability defect, but on second read I think the problem is that this is a for-loop written overly obscurely (too cute). I think a formulation more like this would be substantially clearer, for example:

for (const SystemBase* iter = this; iter != nullptr; iter = iter->get_parent_base()) {
  path_to_root.emplace(iter);
}

In any case, the defect is "its too difficult to tease out what this code is doing".

---

systems/framework/system_base.cc, line 17 at r1 (raw file):

                  path += "::" + node->GetSystemName();
                });
  return path;

Where is this method unit tested from?

---

systems/framework/test/leaf_context_test.cc, line 114 at r1 (raw file):

    if (value == nullptr) return nullptr;  // Not connected.
    auto free_value = dynamic_cast<const FreestandingInputPortValue*>(value);
    EXPECT_NE(free_value, nullptr);

Rule of three -- the prior 5 lines are now identical in all three methods and no longer trivial like previously, so should be factored rather than literally copied.

---

Comments from Reviewable

[sherm1]

Thanks for the comments. I'd be happy to discuss the design f2f if that would be useful.

---

Review status: 13 of 17 files reviewed at latest revision, 8 unresolved discussions.

---

Comments from Reviewable

[sherm1]

BTW I should add that there really is no change here as to who does what with respect to input ports. The logic was twisty before, but if you look carefully you'll see that it went like this:

• user asks System to evaluate an input port
• System asks Context to evaluate the port
• Context looks to see if the port is freestanding and if so returns the value
• Otherwise it calls back to the System's parent Diagram to evaluate the (connected) port
• Diagram finds the connected output port, asks oport's subsystem to evaluate, returns result
• Context passes the result back to the original invoking System

Now:

• user asks System to evaluate an input port
• System asks Context for the freestanding value if there is one, if so returns it
• If not, System asks its parent Diagram to evaluate the connected input port (using a routine of that name)
• Diagram evaluates the connected output as above, result is returned

None of the actual work got moved -- I just removed Context's awkward role as a dispatcher.

---

Review status: 13 of 17 files reviewed at latest revision, 8 unresolved discussions.

---

Comments from Reviewable

[sherm1]

Responded to one small comment with a lot of code :(

---

Review status: 10 of 17 files reviewed at latest revision, 8 unresolved discussions.

---

systems/framework/system.h, line 405 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

GetValue means static cast. It is unclear to me how we know, as of this line, that the abstract_value definitively holds a BasicVector<T> as its erased type.

Similarly, it is unclear to me how we meet our contract "Throws std::bad_cast if the port is not vector-valued". I also do not see a unit test for that promise.

Sigh. These methods were already a mess when I found them -- the documentation lied and got away with it by not having any unit tests! But I guess if I touch them I own them, so PTAL now. I rewrote the documentation to provide some real contracts that are enforced, added a bunch of actually-informative error messages, and unit tested them all (in diagram_test.cc).

The contracts promise to check all preconditions in Debug and state what they will do if they are also checked in Release but don't promise to do so. For now I'm checking everything but I want to be free to measure performance later and drop tests that are too expensive. We can add "OrThrow" methods then if needed.

---

Comments from Reviewable

[jwnimmer-tri]

Small checkpoint.

JN: ... This PR is altering some ... fundamental decisions about how system names are handled ...
MS: ... I didn't intend to change system names now either ...

FYI You're right -- I retract my comment. Some API comment changes confused me about what was (not) being changed.

---

Reviewed 3 of 4 files at r2.
Review status: 13 of 17 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

systems/framework/system.h, line 1923 at r2 (raw file):

  }

  void ThrowNegativeInputPortIndex(const char* func, int port_index) const;

FYI These could be marked noreturn, which might speed things up or at least be helpful documentation.

---

systems/framework/system.cc, line 61 at r2 (raw file):

                  "cannot be evaluated. (Subsystem {})",
                  FmtFunc(func), port, GetSystemPathname()));
}

Methods that are templated on T should not be defined in the system.cc file, or else the user cannot employ their own scalar types. (In case your motivation was that fmt cannot be used in headers, note that #8528 fixed that.)

Possibly these could live in system_base.cc? (I would be fine with them in system.h also.)

---

systems/framework/system_base.h, line 47 at r1 (raw file):

  system. Hand-built systems with no default name at all will be given the
  dummy name "_" (a lone underscore). */
  const std::string& GetSystemName() const final {

It is not clear to a reader of the API comments what the difference in semantics between get_name() and GetSystemName() is to be used for.

After poking around at call sites and uses, I believe:

1. ... that GetSystemName is intended only for human use (such as error messages) and never programatically; and
2. ... that get_name is more like an identifier that can / should be used programatically, something like a coordinate to locate subsystems within a diagram.

Is that correct? In any case, please update documentation somehow.

(FYI Not for this PR, but I suspect that a large refactor to use "name" for log-hint-strings and "identifier" for diagram-string-keys would help clear things up a lot.)

---

systems/framework/test/diagram_test.cc, line 624 at r2 (raw file):

 public:
  using BasicVector<T>::BasicVector;
};

This doesn't meet the BasicVector contract for subclassing (it doesn't clone correctly).

At worst, it should at least comment-disclaim this, but probably it should just add the required override.

(Alternatively, just re-use some existing BasicVector subclass like systems/framework/test_utilities/my_vector.h.)

---

systems/framework/test/diagram_test.cc, line 713 at r2 (raw file):

      std::logic_error,
      ".*EvalInputValue.*expected.*double.*input port.*0.*actual.*int.*");
}

I don't understand why the unit test code is in diagram_test.cc when the implementation is in system.h.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 4 files at r2.
Review status: 14 of 17 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

systems/framework/system.h, line 493 at r2 (raw file):

  /// @pre `port_index` must be non-negative.
  /// @pre `port_index` must designate an existing input port.
  /// @pre the port's value must be exactly of type V.

This "must be exactly" is perhaps too terse for BasicVector subclasses. In that case, I think we don't allow V = MyVector but rather only V = BasicVector, and then use has to do the final downcast. (In general, anything that is type-erased but cloneable would have this same question / problem.)

I'd suggest this either be documented here, or else the body here could learn how to MaybeGetValue of the base clone and then downcast here for the user. (Or maybe MaybeGetValue should be taught that?) Probably document for now with a TODO for MoreMagic(tm) later?

---

systems/framework/system.h, line 495 at r2 (raw file):

  /// @pre the port's value must be exactly of type V.
  ///
  /// The preconditions will be checked in Debug builds but some or both might

BTW s/both/all/

---

systems/framework/system.h, line 1037 at r2 (raw file):

  /// Returns the descriptor of the input port at index @p port_index.
  const InputPortDescriptor<T>& get_input_port(int port_index) const {
    DRAKE_ASSERT_VOID(ThrowNegativeInputPortIndex(__func__, port_index));

This seems wrong. The method isn't ThrowIfNeg -- it's ThrowNeg -- unconditional.

---

systems/framework/system.h, line 1412 at r2 (raw file):

  /// the root %System.
  // Diagram builder ensures we have the same scalar type all the way up.
  const System<T>* get_parent() const {

Unclear why this is public. (It compiles fine as private-only.)

---

systems/framework/system.h, line 1850 at r2 (raw file):

  }

  /// (Internal use only) Diagram systems must reimplement this to evaluate the

Methods that are diagram-specific (do not make sense for leaves) being placed onto the System class is a pretty big design defect. I think that was one of the purposes of the input_port_evaluator_interface that got the axe up above. I do not yet see why this design is better, though I am still in progress with reviewing.

---

systems/framework/system.h, line 1944 at r2 (raw file):

                                             const Context<T>& context,
                                             InputPortIndex port_index) const {
    if (port_index < 0 || port_index >= get_num_input_ports())

BTW Unclear why we're checking < 0 here. Our callers have all done that already. (It could be an ASSERT here I guess?)

---

systems/framework/system.h, line 1955 at r2 (raw file):

    auto free_port_value =
        dynamic_cast<const FreestandingInputPortValue*>(port_value);

This use of dynamic_cast (as a question, rather than assumption) violates GSG. Is it only temporary, per the TODO above?

Also, is it too much churn to fix the TODO as part of this PR? It seems intimately related to the design changes that are being proposed.

---

systems/framework/system.h, line 1971 at r2 (raw file):

  const BasicVector<T>* EvalBasicVectorInputImpl(
      const char* func, const Context<T>& context,
      InputPortIndex port_index) const {

FYI Consider adding an up-front check and better error message if the port wasn't even declared to be vector-valued? (Rather than bailing out only after the AbstractValue dynamic cast fails.)

---

systems/framework/system.h, line 1977 at r2 (raw file):

    // We have a value, is it a BasicVector?
    const BasicVector<T>* basic_value =

FYI Throughout these changes (here and above, etc.), you could add the non-reassignable const if you wanted to (const Foo* const foo = ...;).

---

Comments from Reviewable

[jwnimmer-tri]

I've finished a first pass. I still don't understand the motivation for the evaluation responsibilities design change, and on first read it seems to explode invariants across several classes by making extra stuff public that shouldn't be. I suspect that is some true improvement / problem to be solved going on here, but I don't see it yet, and perhaps there is a more sound way to resolve it than what is proposed here.

---

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

---

systems/framework/context.h, line 401 at r2 (raw file):

  const Context<T>* get_parent() const {
    return parent_;
  }

This used to be properly hidden given the interfaces / abstractions in play. I guess I will have to revisit this choice once I understand the motivation for this design rework.

---

systems/framework/context.h, line 406 at r2 (raw file):

  /// it has never been set with SetInputPortValue().
  /// Asserts if @p index is out of range.
  virtual const InputPortValue* GetInputPortValue(int index) const = 0;

This used to be properly hidden given the interfaces / abstractions in play. I guess I will have to revisit this choice once I understand the motivation for this design rework.

---

systems/framework/system_base.h, line 447 at r2 (raw file):

  // TODO(sherm1) Remove the system type noise once the subsystem path is
  // a fully reliable identifier.
  std::string GetSystemIdString() const {

FYI To me "ID" suggest a programmatic map key (identifier), not an error message string.

---

systems/framework/system_base.h, line 454 at r2 (raw file):

  // Diagram. The enclosing Diagram is needed for interactions between peer
  // subsystems via input and output ports. Aborts if the parent has already
  // been set to something else.

BTW This probably deserves "internal use only" disclaimer or similar in its API docs.

---

systems/framework/system_base.h, line 461 at r2 (raw file):

  // Returns a pointer to the immediately enclosing Diagram
  // if one has been set, otherwise nullptr.

Public methods should use Doxygen style.

---

systems/framework/system_base.h, line 462 at r2 (raw file):

  // Returns a pointer to the immediately enclosing Diagram
  // if one has been set, otherwise nullptr.
  const SystemBase* get_parent_base() const {

Unclear why this is public. (It compiles fine as protected.)

---

Comments from Reviewable

[jwnimmer-tri]

It is a bit hard to reason about, butI'm trying to figure out what improvements take Context out of the Diagram evauation code path, but still maintain our invariants that seem to matter. I think some steps in the right direction are:

1. EvalConnectedSubsystemInputPort really cannot exist on System. Perhaps the input_port_evaluator_interface.h should live on, still be implemented by Diagram, and used by System to call into its parent.
2. Somehow preserve the invariant that when an input port in the Context is connected but not freestanding, that the System corresponding to that Context is within a Diagram -- you can't just set_parent and watch the magic happen, you need to use the diagram workflow. I suspect that this will turn into fixing the TODO where the non-freestanding input port value stuff gets nixed immediately, and then context only (and clearly) represented "disconnected, fixed, connected" as data, rather than trying to pretend to do control flow.

---

Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[sherm1]

I think the problem is actually our idea of what invariants ought to be preserved here. The current architecture makes Diagram a second-class System, unable to acquire resources of its own, and forces us to hide Diagram functionality at the System class level. That is a mistake IMO and was very awkward to work with in caching. The SystemBase class is an attempt to rectify that to some degree and understands that there may be subsystems. We should discuss f2f.

---

Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[sherm1]

Partial response with some code movement that looks like more changes than really happened.

---

Review status: 10 of 18 files reviewed at latest revision, 27 unresolved discussions.

---

systems/framework/system.h, line 495 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW s/both/all/

Done.

---

systems/framework/system.h, line 1037 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This seems wrong. The method isn't ThrowIfNeg -- it's ThrowNeg -- unconditional.

Done.

---

systems/framework/system.h, line 1923 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI These could be marked noreturn, which might speed things up or at least be helpful documentation.

Done. I like that!

---

systems/framework/system.h, line 1944 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Unclear why we're checking < 0 here. Our callers have all done that already. (It could be an ASSERT here I guess?)

Done.

---

systems/framework/system.h, line 1971 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Consider adding an up-front check and better error message if the port wasn't even declared to be vector-valued? (Rather than bailing out only after the AbstractValue dynamic cast fails.)

Done.

---

systems/framework/system.h, line 1977 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Throughout these changes (here and above, etc.), you could add the non-reassignable const if you wanted to (const Foo* const foo = ...;).

Done.

---

systems/framework/system.cc, line 61 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Methods that are templated on T should not be defined in the system.cc file, or else the user cannot employ their own scalar types. (In case your motivation was that fmt cannot be used in headers, note that #8528 fixed that.)

Possibly these could live in system_base.cc? (I would be fine with them in system.h also.)

Done. Moved to system_base.cc. In fact they have to be there soon anyway since the non-typed EvalInputPort stuff moves up to SystemBase.

---

systems/framework/test/diagram_test.cc, line 624 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This doesn't meet the BasicVector contract for subclassing (it doesn't clone correctly).

At worst, it should at least comment-disclaim this, but probably it should just add the required override.

(Alternatively, just re-use some existing BasicVector subclass like systems/framework/test_utilities/my_vector.h.)

Done.

---

systems/framework/test/leaf_context_test.cc, line 114 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Rule of three -- the prior 5 lines are now identical in all three methods and no longer trivial like previously, so should be factored rather than literally copied.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 8 of 8 files at r3.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

---

systems/framework/system_base.h, line 482 at r3 (raw file):

  }

  /** Throws std::logic_error apparently from API function `func` which has

FYI Do you want to promise the more specific out_of_range here? (It's what we throw already.)

---

Comments from Reviewable