Revamp Context values API

sherm1 commented 3 years ago

Background

The current API for setting & getting values from the Context has several problems in practice as discussed in issue #11346. Among those:

GSG-violating snake_case methods that are actually expensive
awkward return types that require several indirections to get at the numeric values, especially for continuous state
overuse of snake_case (even where permitted) making the API look inconsistent to the uninitiated

Proposal summary

Main features:

Never more than one call to get any particular value from a Context
Eliminate cases where user needs to call dangerous methods that return mutable references that can inadvertently bypass cache invalidation
Provide matching CamelCase Setters and Getters, accepting and returning the same simple types
Provide uniform APIs for continuous/discrete/abstract state and numerical/abstract parameters
Hide awkwardness of ContinuousState substructure by having the basic Getter copy to a VectorX
Provide a set of snake_case accessors in the modern style, verb free
Deprecate all of the GSG-violating names

Proposal

Pseudocode for brevity: brackets for optional parameter; left off template declaration for V and Overlay † means method already exists (for plain VectorX).

Setter	Getter
†`SetContinuousState(`_vectorspec`)`	`VectorX GetContinuousState() const` // copies
`SetContinuousStateAt(int index,`_vectorspec`)`	`const T& GetContinousStateAt(int index) const`
`SetContinuousStateAs<Overlay>(const Overlay&)`	`const Overlay& GetContinuousStateAs<Overlay>() const`
---
†`SetDiscreteState([which,]`_vectorspec`)`	`const VectorX& GetDiscreteState([which]) const`
`SetDiscreteStateAt([which,] int index,`_vectorspec`)`	`const T& GetDiscreteStateAt([which,] int index) const`
`SetDiscreteStateAs<Overlay>([which,] const Overlay&)`	`const Overlay& GetDiscreteStateAs<Overlay>([which]) const`
---
†`SetAbstractState<V>([which,] const V&)`	`const V& GetAbstractState<V>([which]) const`
---
`SetNumericParameter([which,]`_vectorspec`)`	`const VectorX& GetNumericParameter([which]) const`
`SetAbstractParameter<V>([which,] const V&)`	`const V& GetAbstractParameter<V>([which]) const`
---
†`SetTime(const T&)`	`const T& time() const`
†`SetAccuracy(optional<double>)`	`const optional<double>& accuracy() const`

_vectorspec ::= const Eigen::Ref<const VectorX<T>>& | initializer_list<T> | const T& (allows e.g. SetContinuousState({1, 2, 3}))
which is optional if 0 is the only possible value

Accessor (const)	Return Type
`time()`	`const T&`
`accuracy()`	`const optional<double>&`
`state()`	`const State<T>&`
`parameters()`	`const Parameters<T>&`

There are more details but I want to stop there to solicit initial thoughts. cc @RussTedrake

sherm1 commented 3 years ago

Here is an alternative that would allow GetContinuousState() to return a reference rather than a copy. But is it too weird to live?

const VectorX<T>& GetContinuousState() const;

Nice signature, but here's how it would have to work:

A DiagramContext would allocate a predefined cache entry containing a VectorX and a Calc() method that gathers the continuous State into contiguous memory.
diagram_context.GetContinuousState() would actually be an "Eval" that would make sure the cache entry had an up to date copy of the continuous state, then it would return a reference to that cache entry
leaf_context.GetContinuousState() would simply return a reference to the live continuous state without copying
a Diagram that happens to have only a single subsystem with continuous State (common I think) could also just return the live reference.

In many cases this would silently produce the optimal behavior but the weird thing is that the referenced state would be live in some cases and a dead copy in others. OTOH the proposal in the issue description above is similar in that the continuous state comes back as a copy while the rest return live references.

Some weirdness is unavoidable because of our attempt to hide that a Diagram's continuous state comprises multiple independent VectorX's. For discrete state we didn't attempt that illusion and consequently we can always return references to the live state as a VectorX. (The proposed reengineering of state in #9171 treats continuous and discrete state similarly as collections of VectorX's.)

The current approach of returning a drake::VectorBase<T>& rather than an Eigen::VectorX<T>& avoids some of this weirdness in exchange for API awkwardness that @RussTedrake says is a pedagogical impediment.

Note: for advanced users we will still have access to the actual underlying types (State, ContinousState, VectorBase, etc.). The question here is how should the beginner's SetContinuousState()/GetContinuousState() pair behave?

sherm1 commented 3 years ago

(I'm working on a PR that will show how the call sites are changed by the new APIs.)

RussTedrake commented 3 years ago

The proposed API above looks great. I guess SetTime() and time() violate your principle of providing matching case setters and getters? Same for SetAccuracy() and accuracy?

RussTedrake commented 3 years ago

Re: VectorX<T> or const VectorX<T>&, I really do think there is value in having the simple version look simple. I don't have a strong preference between these, too. Keep in mind that the python versions are going to do the copy in either case, (if I understand correctly), since it will convert to the numpy array, and honestly it's the python API that will be used the most by the non-experts.

RobotLocomotion / drake