State variable abstraction needs an upgrade

[sherm1]

This issue is extracted from the discussion in #6998.

Currently we have the concepts of "configuration" and "velocity" variables mixed up with "continuous" and "discrete". The generalized configuration variables q (and generalized velocities v) are still configuration (velocity) whether updated continuously or discretely. Configuration variables contribute to poses and potential energy, velocity variables contribute to motion and kinetic energy -- that's important to know for many reasons (caching, for example) and we are losing that when we need to use discrete variables for q and v.

Here is the basic proposal from #6998 (from me, @edrumwri, @mitiguy):

• There are numerical states and abstract states.
• Numerical states come in "groups" that are BasicVectors (DiscreteStates are like this now).

Groups may be assigned certain properties:

• group contains: configuration/velocity/mass/general variables
• has_time_derivatives
• is_second_order_pair (qᵢ vᵢ)

Otherwise, there would be no distinction between continuous & discrete variables. (Diagram-level state would just be a bigger collection of BasicVector state groups.) This reflects nicely the fact that the physical system may enable continuous simulation, but it does not require continuous simulation; that is up to the numerical algorithm being applied to the System. Potential energy would automatically depend on configuration and mass variables, KE on config, velocity, and mass variables regardless of whether those are being treated continuously at the moment.

Note: consider whether is_parameter (or is_time_varying) should be a property also, so that continuous/discrete/parameter could just be three flavors of the same object, shrinking the code considerably.

[tri-ltyyu]

Formulating general fixed-point constraints are nearly impossible to get right without this feature, hence keeping priority.

[amcastro-tri]

Update: the kinematics cache work around to this caused a very difficult to track bug, see #10880 (closed since this issue tracks the problem, but still relevant to show the level of confusion).

[sherm1]

Per discussion with @RussTedrake, for System analysis it is important to know the complete state so currently our analysis tools refuse to work with any System that has abstract state. However, Systems often use abstract state for non-physical things (results intended only for output or debugging, the current collection of geometries in SceneGraph). Problem: we want people to use abstract state where it makes sense, but don't want non-physical abstract states to prevent analysis.

Idea (from discussion with @SeanCurtis-TRI): when revamping the state as envisioned in this issue, support a tag like "not_physical" [think up a better name] for state variables. Then analysis tools can refuse to work with Systems that contain mysterious "physical" abstract state, while not worrying about uses that are essentially just bookkeeping.

[jwnimmer-tri]

Possibly more useful would be to tag such tainted state, but then guard based on which operations it taints (i.e., cache entries). So if it only taints a debugging output port, and that port is not connected, then it can be ignored for analysis. Basically, a sparsity concern, rather than trying to define what "non-physical" means. In fact, I think already right now, it should be practical to ask which other state + which outputs depend on any/all abstract state.

Edited 2019-04-17: Aside: #11201 uses such an idea, by inferring input-output sparsity from the cache trackers in a Context.

[jwnimmer-tri]

I think there's an important "be conservative in what the framework assumes" gotcha lurking in this feature. I think it's a current bug right now (but maybe hasn't bitten anyone), but one that may get worse with the improvements envisioned in this issue.

Right now, the configuration_ticket and kinematics_ticket do not depend on z (the "misc" continuous state). Within the continuous state, those tickets only depend on q (or {q,v}, resp). In other words, they only depend on what the user has explicitly labeled as configuration continuous state and velocity continuous state.

If the user just calls the simple and boring DeclareContinuousState(2) then the framework assumes that xc is exclusively misc state z. That is fine -- we should not presume that it's necessarily kinematic state -- but when setting up cache invalidation defaults neither should we presume that it's not kinematic state! We should presume nothing either way.

In a recent PR I almost declared that a system's velocity output port depends only on the kinematics ticket, but as it turns out my system used a named_vector for its xc so it was declared as misc continuous state, so the kinematics_ticket did not cover any of my state. Disaster!

A similar problem will happen when we introduce discrete state group labels for kinematics. If the user has an existing system that uses the old APIs, we cannot infer whether their discrete state is kinematic or not. We have to still include it under the kinematics_ticket, until they port to our newer APIs and/or opt-in to extra arguments to specify the labels.

[sherm1]

I'm working on the spec for this which I'll share as soon as it is somewhat coherent.

On Wed, Apr 17, 2019, 6:29 PM Jeremy Nimmer notifications@github.com wrote:

I think there's an important "be conservative in what the framework assumes" gotcha lurking in this feature. I think it's a current bug right now (but maybe hasn't bitten anyone), but one that may get worse with the improvements envisioned in this issue.

Right now, the configuration_ticket and kinematics_ticket do not depend on z (the "misc" continuous state). Within the continuous state, those tickets only depend on q (or {q,v}, resp). In other words, they only depend on what the user has explicitly labeled as configuration continuous state and velocity continuous state.

If the user just calls the simple and boring DeclareContinuousState(2) then the framework assumes that xc is exclusively misc state z. That is fine -- we should not presume that it's necessarily kinematic state -- but when setting up cache invalidation defaults neither should we presume that it's not kinematic state! We should presume nothing either way.

In a recent PR I almost declared that a system's velocity output port depends only on the kinematics ticket, but as it turns out my system used a named_vector for is xc so it was declared as misc continuous state, so the kinematics_ticket did not cover any of my state. Disaster!

A similar problem will happen when we introduce discrete state group labels for q,v,z. If the user has an existing system that uses the old APIs, we cannot infer whether their discrete state is kinematic or not. We have to still include it under the kinematics_ticket, until they port to our newer APIs and/or opt-in to extra arguments to specify the labels.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/RobotLocomotion/drake/issues/9171#issuecomment-484320939, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7GBUAM3LUIEUUQTGJYFZLPQ7FF7ANCNFSM4FLJJQWA .

--

Confidential or protected information may be contained in this email and/or attachment. Unless otherwise marked, all TRI email communications are considered "PROTECTED" and should not be shared or distributed. Thank you.

[sherm1]

Status: design doc in progress; unclear how to trade off various goals; will discuss with team at onsite.

[sherm1]

Discussed with team during onsite. Agreed on a reasonable scheme. Writeup in progress.

[sherm1]

Status: a little progress this week but didn't have much time. Will attempt to be done with version 2 this week.

[sherm1]

Progress: Discussed State 2.0 interaction with the Event system with Evan, added an Event section to the WIP doc.

Plans: make sure I understand Russ's analysis needs so that the new State/Events architecture isn't an impediment.

[sherm1]

Updated the State 2.0 doc with a new proposal -- ready for another look @RussTedrake @jwnimmer-tri @edrumwri @amcastro-tri @EricCousineau-TRI.

Although this proposal attempts to address a bunch of related problems, the kickoff motivation for me was that I can't tell which variables affect configuration so cache entries in MBP get invalidated too often. That could be addressed for the most egregious case (MBP) without much re-engineering (e.g. allow addition of individual variables to the configuration_ticket()). Unless everyone is excited about re-engineering Drake variables and events as proposed or otherwise, we should consider deferring the larger project and just fixing the most-pressing items.

[jwnimmer-tri]

I think it would help if the proposal included (pseudo-)code that shows the new APIs in situ -- basically porting a few current use cases to the new APIs. Both for declaring a System as well as using the new APIs (e.g., interrogating a Context).

[sherm1]

I agree -- some sketchy code is my next step.

[sherm1]

On hold until recovered from various team members going on vacation.