RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.33k stars 1.26k forks source link

Users don't know when they need to define initialization events #12649

Open sherm1 opened 4 years ago

sherm1 commented 4 years ago

I've seen many user-defined Systems that were broken due to lack of an initialization event, resulting in invalid values at the t=0 start of simulation (see #12643 for a recent example). I'm not sure what we could do to avoid these problems. I'm not sure more documentation would do it. More examples would help I think. Is there any automation we could provide?

jwnimmer-tri commented 4 years ago

It might help if you could list the places that currently require / define such events, so we could try to measure what they all have in common or how we could automatically check them (by removing the event and seeing how we can make it gripe).

jwnimmer-tri commented 2 years ago

In #13776, we've observed that the only two uses of initialization events throughout all of Drake and all of Anzu are both defective.

My hypothesis is that initialization events have no valid use cases of any kind. Anyone who has tried to use them is actually making an incorrect model of the world, or at least of their State, in some fashion. We could document that as a resolution to this issue, but I'd much rather deprecate and remove this event type entirely.

What do you think @sherm1?

sherm1 commented 2 years ago

I'm still a fan of Initialization Events. They are a necessary part of a general discrete event facility. I'm not sure why they are causing so much confusion in practice.

jwnimmer-tri commented 2 years ago

Having at least one motivating example available would probably be a good starting point. I've never been able to conjure one up.

edrumwri commented 2 years ago

We actually could not get by without initialization events! A bit hard to explain our use case, but it's a perfect storm of Monte Carlo simulation plus real-time plus IPC.

jwnimmer-tri commented 2 years ago

Today, another user was confused by initialization events.

If nobody can provide even a single example in this issue discussion where initialization events make sense, then we need to deprecate and remove them.

If we decide to keep them, then we'll need to rework most of our examples to stop calling simulator.Initialize() gratuitously. It should only be called for some acute and unusual reason (e.g., resetting an already-simulated context back to zero), not as boilerplate as part of everyday use of the simulator.

sherm1 commented 2 years ago

I checked with @edrumwri -- there are currently 9 Initialization Events in the Dextrous Robotics code base. Evan, could you say a little about how they are used so Jeremy will stop trying to remove them :)

edrumwri commented 2 years ago

The most common use case within the Dextrous code base happens when we have some abstract state that is initialized based on the value of one or more Parameters. Since the user sets the Parameters after the Context has been created, we need the abstract state to be re-initialized using the latest Parameter settings. I'd prefer not to go into more detail, but I'll repeat that we are depending on this mechanism.

jwnimmer-tri commented 2 years ago

Okay, I think that's a clear description of what's happening. I'll work on creating a toy example that demonstrates that situation, and then we can switch to discussing that example specifically.

jwnimmer-tri commented 2 years ago

FYI recently Russ added #17824 using an initialization event.

The common theme here seems to be the state update initialization events end up being useful, so the call to action here will probably be to better explain and demonstrate that feature, so that users can understand it. (And also explain the risks of subsystem initialization event ordering, in case the update handler depends on an input port, where the outputting system also declares an initialization event.)

In prior work, people were using initialization triggers for publish events, which I still don't think we have any valid use case for. We'll probably want to steer people away from that use case.

EricCousineau-TRI commented 1 year ago

I guess I'm still generally confused by the two competing features (initialization events vs. scheduled discrete updates). I have some "hack" initialization in Python code, and wanted to see if it might be worth upgrading to what I believe to be proper initialization a la #13540 (\cc @sammy-tri w.r.t. robot hub maybe).

In #18356, I tried to distill the following into simplified example (relates #11403). Rather than distilling an understandable concept, it encodes my confusion :sweat_smile: https://github.com/RobotLocomotion/drake/blob/4c4b9f7c829f21822c57267f01916851bc7d15c9/manipulation/kuka_iiwa/iiwa_command_receiver.cc#L88-L90 I believe I understand discrete updates and dynamical systems should happen in a certain order, but I would expect affordances for initialization to (a) be the same and (b) somehow propagate at initialization. (There's also a (c) about the confusion for kEps, but mayhaps less surprising.)

Code permalink: https://github.com/RobotLocomotion/drake/blob/66d0457facff9a2078addf595a7b064abdf59cfb/systems/framework/test/leaf_system_discrete_initialization_test.cc#L77-L105

My takeaway is I'm going to continue hacking initialization by hacking state into undeclared state or cache (eww, but OK) and monitoring if time goes backwards (no variable step integrators AFAIK). Nasty, but at least that I (somewhat) understand it all.

Happy turkey week hacking! :turkey: :crab:

sherm1 commented 1 year ago

Admittedly the details of event handling are complicated and hard to keep in mind. However, the test code behavior you provided looks exactly right to me. Evan and I worked for months with Russ to get the behavior right and to document it in excruciating detail. There are two rather beautiful documents (IMHO) that might help clear this up:

In the second document, looks closely at when discrete updates occur -- they are always applied at the beginning of a step. So if they trigger at the end of a step via AdvanceTo(trigger_time) they will be applied at the start of the next step as you observed. BTW, in contrast, Publish events occur at the end of a step. That may sound odd, but believe me -- all other alternative behaviors are worse!

Please LMK if that doesn't help.

EricCousineau-TRI commented 1 year ago

Thanks, yup all of that helps!

I think I'm still a bit hung up on what kind of initialization events there are; I think having the doc of "here's the proper / suggested way to initialize a system w/ discrete or abstract state" per issue overview would definitely help alleviate it.

Per above, the pattern of "use DoCalcNextUpdateTime() to execute t=now" seems like it was an improvement over prior art, but it's still unclear to me if this is the best pattern for initialization w.r.t. other event types; additionally, as a user I don't love ZOH's LatchInputPortToState() and other similar mechanisms that may happen - I have a few different usages of "here's a post-Init() functor to execute after I build a diagram and initialize its context" that I've used and don't love.

I've mentioned these more concretely in the toy PR #18356.

sherm1 commented 1 year ago

Replied in #18356. I'm thinking that our ZOH's lack of an Initialization event that would latch u -> y is causing unnecessary confusion.