Implements System<T>::ExecuteInitializationEvents()

[RussTedrake]

and uses it in MeshcatPoseSliders and JointSliders.

+@sherm1 for feature review, please.

---

This change is 

[jwnimmer-tri]

FYI prior art:

• systems: Add DispatchInitializeEvents(System) #10015 (comment)
• systems: Add PublishInitializationEvents #13760

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers

---

systems/framework/system.cc line 488 at r1 (raw file):

  GetInitializationEvents(*context, init_events.get());
  if (init_events->get_publish_events().HasEvents()) {

nit These events are executed in the wrong relative order.

See here:

drake/systems/analysis/simulator.cc

Line 84 in 37d6c59

// Do unrestricted updates first.

drake/systems/analysis/simulator.cc

Line 233 in 37d6c59

// The general policy here is to do actions in decreasing order of

[RussTedrake]

I'm fine with a goal of having visualizers not require initialization. But we have other systems that use initialization events, too, and I think this workflow should be supported.

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

---

systems/framework/system.cc line 488 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit These events are executed in the wrong relative order.

See here:

drake/systems/analysis/simulator.cc

Line 84 in 37d6c59

// Do unrestricted updates first.

drake/systems/analysis/simulator.cc

Line 233 in 37d6c59

// The general policy here is to do actions in decreasing order of

Done. Good catch.

[jwnimmer-tri]

The links were not meant to imply that this API is inappropriate.

[jwnimmer-tri]

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

---

systems/framework/system.cc line 488 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Done. Good catch.

There were no test changes in the r2 push though, which means the unit tests are lacking (insufficient to flag this likely implementation defect).

I'll let Sherm weigh in on whether the Simulator implementation just call this new method as a subroutine, which might be a good way to help test-cover it thoroughly.

[sherm1]

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

---

systems/framework/system.cc line 488 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

There were no test changes in the r2 push though, which means the unit tests are lacking (insufficient to flag this likely implementation defect).

I'll let Sherm weigh in on whether the Simulator implementation just call this new method as a subroutine, which might be a good way to help test-cover it thoroughly.

My 2 cents: It would be great to call this from Initialize() -- that would provide good testing, avoid duplication, and ensure perfectly matching behavior forever. However, the functionality is subtly different: the Simulator's Initialize has to publish early per-step and periodic events and track some statistics. So combining them probably requires pulling out some common functionality into a gawky private method that could be invoked from both places. Might not be worth it, but in that case a strong warning here that this must match the Initialize() code (esp. the unrestricted/discrete/publish order) would be worthwhile.

(P.S. I'm on vacation this week and will shortly be deprived of net access for a few days in the wilderness of Big Sur.)

[jwnimmer-tri]

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

---

systems/framework/system.cc line 488 at r1 (raw file):

... in that case a strong warning here that this must match the Initialize() code (esp. the unrestricted/discrete/publish order) would be worthwhile.

That would work for me.

I'm on vacation this week.

If there's a desire to have the FooSliders classes process initialization events in the meantime, I suggest the easiest path forward here would be to call Simulator::Initialize in the FooSliders::Run functions for now, and then circle back to this core systems PR later to cut that dependency.

[RussTedrake]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers

---

systems/framework/system.cc line 488 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

... in that case a strong warning here that this must match the Initialize() code (esp. the unrestricted/discrete/publish order) would be worthwhile.

That would work for me.

I'm on vacation this week.

If there's a desire to have the FooSliders classes process initialization events in the meantime, I suggest the easiest path forward here would be to call Simulator::Initialize in the FooSliders::Run functions for now, and then circle back to this core systems PR later to cut that dependency.

Done. If #19578 lands then I'm unblocked on my main project. I don't mind waiting on this PR if need be.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers

---

systems/framework/system.cc line 488 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Done. If #19578 lands then I'm unblocked on my main project. I don't mind waiting on this PR if need be.

I think the remaining question is whether we want to insist on a unit test of this function that would detect when the event handlers were processed in the wrong order (e.g., with publishes happening before updates). I still lean towards "yes" but I'll defer to Sherm for the final word.

[RussTedrake]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers

---

systems/framework/system.cc line 488 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think the remaining question is whether we want to insist on a unit test of this function that would detect when the event handlers were processed in the wrong order (e.g., with publishes happening before updates). I still lean towards "yes" but I'll defer to Sherm for the final word.

Perhaps the strong warning is enough? @sherm1 -- welcome back. waiting for your thoughts.

[sherm1]

Feature
+@jwnimmer-tri for platform review please, since I'm tomorrow's

Reviewed 7 of 9 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

---

systems/framework/system.h line 733 at r3 (raw file):

  GetInitializationEvents(). The method allocates temporary storage to perform
  the updates, and is intended only as a convenience method for callers who do
  not want to use the full Simulator workflow. */

minor: Please add something like: "Note that this is not fully equivalent to Simulator::Initialize() because only initialization events are handled here while Simulator::Initialize() also processes time-0 periodic and per-step events."

---

systems/framework/system.cc line 488 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Perhaps the strong warning is enough? @sherm1 -- welcome back. waiting for your thoughts.

IMO the small function + warning is enough.

[jwnimmer-tri]

Reviewed 7 of 9 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

---

multibody/meshcat/joint_sliders.cc line 282 at r3 (raw file):

  const auto start_time = Clock::now();

  diagram.ExecuteInitializationEvents(root_context.get());

If I delete this line of code, I think maybe no tests will fail? If that's true, then we are missing a unit test case on this class.

---

systems/framework/system.cc line 494 at r3 (raw file):

    CalcUnrestrictedUpdate(*context,
                           init_events->get_unrestricted_update_events(),
                           state.get());

It seems to me like the new (post-initialization) state and discrete_updates values are complete ignored?

The simulator applies the updates after calculating them, e.g.,

    system_.ApplyUnrestrictedUpdate(events, unrestricted_updates_.get(),
        context_.get());

---

systems/framework/test/leaf_system_test.cc line 2884 at r3 (raw file):

  EXPECT_TRUE(dut.get_unres_update_init());

  // Now again with the ExecuteInitializationEvents method.

Working

In test cases, having two copies of local variables foo vs foo2 makes it way too easy to accidentally typo one name versus the other, and end up checking the wrong object(s) for their properties, leading to missing test coverage. Spotting a one-character typo during review is too difficult.

I'll provide a patch that carves this out into a separate test case.

[jwnimmer-tri]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

---

systems/framework/test/leaf_system_test.cc line 2884 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

In test cases, having two copies of local variables foo vs foo2 makes it way too easy to accidentally typo one name versus the other, and end up checking the wrong object(s) for their properties, leading to missing test coverage. Spotting a one-character typo during review is too difficult.

I'll provide a patch that carves this out into a separate test case.

See https://gist.github.com/jwnimmer-tri/b6b7bb9cd3479d59352d1f5909698545.

It just extracts the helper class to be outside of the test function, and splits into two gtests.

[RussTedrake]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

---

multibody/meshcat/joint_sliders.cc line 282 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If I delete this line of code, I think maybe no tests will fail? If that's true, then we are missing a unit test case on this class.

Done. I hit the rule of three, so had to touch a few files on this one.

---

systems/framework/system.h line 733 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: Please add something like: "Note that this is not fully equivalent to Simulator::Initialize() because only initialization events are handled here while Simulator::Initialize() also processes time-0 periodic and per-step events."

Done.

---

systems/framework/system.cc line 494 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It seems to me like the new (post-initialization) state and discrete_updates values are complete ignored?

The simulator applies the updates after calculating them, e.g.,

    system_.ApplyUnrestrictedUpdate(events, unrestricted_updates_.get(),
        context_.get());

Done. Yikes. Good catch. Thanks.

---

systems/framework/test/leaf_system_test.cc line 2884 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

See https://gist.github.com/jwnimmer-tri/b6b7bb9cd3479d59352d1f5909698545.

It just extracts the helper class to be outside of the test function, and splits into two gtests.

Done.

[sherm1]

Reviewed 12 of 12 files at r4, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

---

systems/framework/system.h line 736 at r4 (raw file):

  Note that this is not fully equivalent to Simulator::Initialize() because
  *only* initialization events are handled here, while Simulator::Initialize()

minor: the * at beginning of line confuses Doxygen (it thinks that's one of those archaic comment formatting styles and eats the first *:

Use _only_ instead. I would suggest always using underscores instead of asterisks to avoid this problem -- even if it starts out OK a little reflowing can push the asterisk to BoL later.

[RussTedrake]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

---

systems/framework/system.h line 736 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: the * at beginning of line confuses Doxygen (it thinks that's one of those archaic comment formatting styles and eats the first *:

Use _only_ instead. I would suggest always using underscores instead of asterisks to avoid this problem -- even if it starts out OK a little reflowing can push the asterisk to BoL later.

Done. Thanks for catching it.

[sherm1]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

[jwnimmer-tri]

Reviewed 11 of 12 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 5 unresolved discussions

---

systems/framework/system.h line 738 at r5 (raw file):

  _only_ initialization events are handled here, while Simulator::Initialize()
  also processes other events associated with time zero.
  */

nit This file places */ at the end of line, not on a fresh line.

Suggestion:

  also processes other events associated with time zero. */

---

systems/framework/test/leaf_system_test.cc line 2850 at r5 (raw file):

  EXPECT_TRUE(dut.get_dis_update_init());
  EXPECT_TRUE(dut.get_unres_update_init());
  EXPECT_TRUE(context->get_discrete_state_vector()[0]);

Checking the true/false truth value of the discrete state floating-point value does not clearly communicate what's happening.

Suggestion:

  EXPECT_EQ(context->get_discrete_state_vector()[0], 1.23);

---

systems/framework/test/leaf_system_test.cc line 2851 at r5 (raw file):

  EXPECT_TRUE(dut.get_unres_update_init());
  EXPECT_TRUE(context->get_discrete_state_vector()[0]);
  EXPECT_NE(context->get_abstract_state<bool>(0), 0.0);

Comparing true to != 0.0 does not clearly communicate what's happening.

Suggestion:

EXPECT_TRUE(context->get_abstract_state<bool>(0));

---

systems/framework/test_utilities/BUILD.bazel line 29 at r5 (raw file):

    hdrs = ["initialization_test_system.h"],
    deps = [
        "//systems/framework",

For edit-test responsiveness, we shouldn't add unnecessary dependencies to our framework package's own unit tests. Prior to this PR, the utilities library depended only practically no framework code. Now it depends on all of it.

To resolve this, please edit drake/systems/framework/BUILD.bazel so that instead of using the roll-up "//systems/framework/test_utilities" anywhere, it only depends on the exact components it needs.

Note that the roll-up "//systems/framework/test_utilities" is fine for other packages to use (e.g., visualization) since they are already pulling in the whole framework. It's only the framework's own BUILD file where this matters.

---

systems/framework/test_utilities/initialization_test_system.h line 10 at r5 (raw file):

namespace systems {

class InitializationTestSystem : public LeafSystem<double> {

nit Now that this is project-wide, it needs a class overview API doc summarizing what it accomplishes.

[RussTedrake]

Reviewable status: 1 unresolved discussion

---

systems/framework/system.h line 738 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit This file places */ at the end of line, not on a fresh line.

Done

---

systems/framework/test/leaf_system_test.cc line 2850 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Checking the true/false truth value of the discrete state floating-point value does not clearly communicate what's happening.

Done.

---

systems/framework/test/leaf_system_test.cc line 2851 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Comparing true to != 0.0 does not clearly communicate what's happening.

Done.

---

systems/framework/test_utilities/BUILD.bazel line 29 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For edit-test responsiveness, we shouldn't add unnecessary dependencies to our framework package's own unit tests. Prior to this PR, the utilities library depended only practically no framework code. Now it depends on all of it.

To resolve this, please edit drake/systems/framework/BUILD.bazel so that instead of using the roll-up "//systems/framework/test_utilities" anywhere, it only depends on the exact components it needs.

Note that the roll-up "//systems/framework/test_utilities" is fine for other packages to use (e.g., visualization) since they are already pulling in the whole framework. It's only the framework's own BUILD file where this matters.

Done.

---

systems/framework/test_utilities/initialization_test_system.h line 10 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Now that this is project-wide, it needs a class overview API doc summarizing what it accomplishes.

Done.

[jwnimmer-tri]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),sherm1(platform)

[sherm1]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),sherm1(platform)