primitives: Introduce VectorLogSink

[rpoyner-tri]

Relevant to: #10228

This is step 2 of 4 to deprecate and replace SignalLogger and SignalLog
with a logging system that avoids threading hazards (SignalLogger) and
slow memory management (SignalLog).

VectorLogSink will eventually replace SignalLogger. The chief
improvement is relocation of log storage from member data in a System(!)
to storage per-context via a scratch cache entry. The direct-to-log
accessors and mutators are removed, but access is clarified and still
convenient via {Get,Find}[Mutable]Log methods.

---

This change is 

[rpoyner-tri]

+@sherm1 for feature review.

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @sherm1)

[jwnimmer-tri]

In case you haven't seen it yet, checkout how LcmPublisherSystem uses TriggerTypeSet in its (factory) constructor to allow the user to configure the publication triggers. It was a hard fought battle to eventually settle on that design as the most ergonomic choice, versus the various boutique member functions like you have here (and like LcmPublisherSystem used to use).

[rpoyner-tri]

Cool. 👀

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @sherm1)

[rpoyner-tri]

Hm, so adopting that would be a rule-of-2 usage. Copy or factor properly?

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @sherm1)

[jwnimmer-tri]

If you're interested in factoring the sugar back down into LeafSystem so that users can declare one publisher function with multiple triggers, I'd support your initiative! It's a major shortcoming IMO. (Just ask @SeanCurtis-TRI about his recent battles with legacy pydrake visualizers!)

I'd also be content if you wanted to bash on regardless here, with a TODO to refactor once we reach rule-of-3.

[rpoyner-tri]

r2 is the TriggerTypeSet style API, aped from LcmPublisherSystem.

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @sherm1)

[sherm1]

Nice! feature pending a few minor comments, ptal

Reviewed 1 of 4 files at r1, 3 of 3 files at r2.
Reviewable status: 12 unresolved discussions, needs at least two assigned reviewers (waiting on @rpoyner-tri)

---

systems/primitives/vector_log_sink.h, line 27 at r2 (raw file):

/// A discrete sink block which logs its vector-valued input to per-context
/// memory. This data is then retrievable (e.g. after a simulation).  The

minor: should note strongly that logged data MUST NOT be used to modify the behavior of a simulation. Also I'm not sure there is any valid reason to access the log's contents prior to the end of the simulation -- maybe just say it is retrievable after a simulation completes to avoid trouble?

BTW You could add a technical note somewhere below explaining that the log is not stored in the System's State so should not be considered part of the state. That allows us to use Publish() as the handler rather than one of the state update handlers.

---

systems/primitives/vector_log_sink.h, line 41 at r2 (raw file):

/// at "forced publish" events, meaning at explicit calls to System::Publish().
/// The Simulator's "publish every time step" option also results in forced
/// publish events, so should be disabled if you want to control logging

minor: add "(the default)" after "disabled" so reader won't think they have to take some action here

---

systems/primitives/vector_log_sink.h, line 46 at r2 (raw file):

/// @see LogVectorOutput() for a convenient way to add %logging to a Diagram.
/// @see Simulator::set_publish_every_time_step()
/// @see Simulator::set_publish_at_initialization()

BTW these Simulator features are anachronistic and almost always the wrong thing to do (per-step Publish Events are the preferred replacement). I would rather that they not get mentioned here -- seems too encouraging.

---

systems/primitives/vector_log_sink.h, line 71 at r2 (raw file):

  /// to the number of rows of the data matrix.
  /// @param publish_period Period that messages will be published (optional).
  /// If the publish period is zero, VectorLogSink will use per-step

nit: add "or missing" after "zero"

---

systems/primitives/vector_log_sink.h, line 78 at r2 (raw file):

  explicit VectorLogSink(int input_size, double publish_period = 0.0);

  /// Constructs the vector log sink.

BTW doxygen would be better if the brief descriptions were different for the two constructors, e.g. "Constructs the vector log sink with a specified trigger set."

---

systems/primitives/vector_log_sink.h, line 103 at r2 (raw file):

  /// Access the log within this component's context.
  const VectorLog<T>& GetLog(const Context<T>& context) const;

minor: what happens if this isn't a Context for this System? Should document error conditions or at least prereqs (also for the other 3 methods).

---

systems/primitives/vector_log_sink.h, line 109 at r2 (raw file):

  /// Access the log within a containing root context.
  const VectorLog<T>& FindLog(const Context<T>& context) const;

BTW consider calling the parameter root_context so it doesn't look so much like the previous one (also below).

An alternative would be to allow either the log context or root context to be supplied here - then FindLog() could always be used, with GetLog() removed or left in as a slightly-faster alternative.

---

systems/primitives/vector_log_sink.h, line 130 at r2 (raw file):

/// LogVectorOutput provides a convenience function for adding a VectorLogSink,
/// initialized to the correct size, and connected to an output in a

nit: output -> output port

---

systems/primitives/vector_log_sink.h, line 137 at r2 (raw file):

/// @param builder the diagram builder.
/// @param publish_period Period that messages will be published (optional).
/// If the publish period is zero, VectorLogSink will use per-step

nit: zero -> zero or missing

---

systems/primitives/vector_log_sink.cc, line 35 at r2 (raw file):

          "log", ValueProducer(
              // TODO(jwnimmer-tri) Improve ValueProducer constructor sugar.
              internal::AbstractValueCloner(VectorLog<T>(input_size)),

minor: AbstractValueCloner and TODO not needed here

---

systems/primitives/test/vector_log_sink_test.cc, line 126 at r2 (raw file):

  const auto& log = logger->FindLog(simulator.get_context());
  EXPECT_EQ(log.num_samples(), 11);

FYI this is playing with fire, since it depends on roundoff details when accumulating the non-representable 0.1s periods. Consider instead using a perfectly-representable base-2 period like 0.125. Then the period arithmetic will be exact and you'll get 9 samples no matter what fp instructions get generated.

---

systems/primitives/test/vector_log_sink_test.cc, line 171 at r2 (raw file):

  EXPECT_TRUE(is_autodiffxd_convertible(*diagram));
}

minor: consider adding a test to verify that a force trigger does not cause a log entry when the general form is used but the set does not contain kForced.

[rpoyner-tri]

Reviewable status: 12 unresolved discussions, needs at least two assigned reviewers (waiting on @rpoyner-tri)

---

systems/primitives/vector_log_sink.h, line 103 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: what happens if this isn't a Context for this System? Should document error conditions or at least prereqs (also for the other 3 methods).

Funny story: as shown in r2, these only throw in debug builds, thanks to only hitting context validation in CacheEntry, which encloses it check in DRAKE_ASSERT_VOID(), presumably for performance reasons. Added a local context validation call, and also added test cases for possible error conditions.

[sherm1]

Thanks for the updates --
+@soonho-tri for platform review per rotation, please

Reviewed 1 of 3 files at r3, 2 of 2 files at r4.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee soonho-tri(platform) (waiting on @rpoyner-tri and @soonho-tri)

---

systems/primitives/vector_log_sink.h, line 38 at r4 (raw file):

/// should not be considered part of that state. This distinction allows the
/// implementation to use `Publish()` as the event handler, rather than one of
/// the state-aware handlers.

nit: consider "state-modifying" instead (Publish is aware of the state but can't change it).

[soonho-tri]

Reviewed 1 of 4 files at r1, 1 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: 1 unresolved discussion (waiting on @rpoyner-tri)

---

systems/primitives/vector_log_sink.h, line 96 at r5 (raw file):

  /// kPeriodic.
  /// @pre publish_period is non-negative.
  /// @pre publish_triggers contains a subset of {kForced, kPeriodic, kPerStep}.

I think we can remove this line as we have Will throw an error if empty or if unsupported types are provided. above.

[soonho-tri]

Reviewed 1 of 2 files at r4.
Reviewable status: 1 unresolved discussion (waiting on @rpoyner-tri)

[rpoyner-tri]

Reviewable status: 1 unresolved discussion (waiting on @soonho-tri)

---

systems/primitives/vector_log_sink.h, line 96 at r5 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

I think we can remove this line as we have Will throw an error if empty or if unsupported types are provided. above.

Done.

[soonho-tri]

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all discussions resolved, LGTM from assignees soonho-tri(platform),sherm1(platform) (waiting on @rpoyner-tri)