RobotLocomotion / drake

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

ContinuousState should be a BasicVector #6998

Open RussTedrake opened 6 years ago

RussTedrake commented 6 years ago

As I just whined about in #6997, the fact that ContinuousState is often not a BasicVector is presenting itself continually as a pain point in the API.

This becomes a pain point because every reference to the continuous state as a vector must use CopyToVector instead of get_value(). This is inefficient, and presents an inconsistent API to the user (DiscreteState can be accessed with get_value because they are BasicVectors).

Furthermore, many systems copy their state to the output. DeclareOutputPort expects a BasicVector. But that means that I can often directly set the output port to be the same (derived) type as the actual state... it must be a BasicVector copy of it.

The original reason for this, I believe, was to support the q,v,z breakdown, but in fact we support that anyhow (for instance, PendulumState is a BasicVector which declares itself to have 1 position and 1 velocity).

cc @sherm1 all, please let me know if I'm wrong about any of the above.

RussTedrake commented 6 years ago

spent more time on this and understand better now how deep the rabbit-hole is. The complexity comes not from the {q,v,z} but from assembling the ContinuousState in a Diagram (as Supervectors pointing to the subsystem state).

Just to stick it to me, LeafSystem currently enforces (and then assumes in other places) that ContinuousState must be a BasicVector.

This makes it nearly impossible to accomplish my goal in #6938. For example, the LuenbergerObserver would like to DeclareContinuousState with the same (runtime derived) type as the observed system... but it cannot, because things explode if you pass in a Diagram as the system. This means that we can never address the context of the observer using the carefully constructed types from the original system (a feature which is very desirable, e.g. for initializing the state of the observer).

I don't see a great fix yet. But it's worth discussing sooner rather than later. @sherm1's idea the other day is that we could use a std::vector of 'BasicVector's to represent the ContinuousState, like we do with the DiscreteState. Perhaps that's the right solution.

sherm1 commented 6 years ago

I'm also suffering at the hands of ContinuousState in the caching code at the moment. Because we use it both for the state q,v,z and their derivatives qdot,vdot,zdot I need to put one of these as the AbstractValue for the time derivatives cache entry. But, ContinuousState is not cloneable so is ineligible. I'm adding clone but in the process also fell into the diagram rabbit hole. I am reworking it to provide a deep clone as we do for Discrete and Abstract states (in a backwards-compatible way).

Another problem created by the current implementation is the assumption that the position and velocity (2nd order) variables will be continuous. That is of course not true for a time-stepped discrete system, so these have to be shoehorned into the discrete variables with no help from the framework. If we do a serious re-engineering of ContinuousState to make it easier to work with, we should also consider decoupling the identification of kinematic variables with the continuous state -- ContinuousState and DiscreteState could both consist of BasicVector groups, with certain group pairs designated as (q,v) so that (for example) the dependencies for PE and KE calculations can be determined automatically (at the moment I have to assume they are dependent on q and v but that only works for continuous systems). The System method that maps v to qdot is needed regardless of whether these are continuous or discrete, but currently thinks it knows to find v in the ContinuousState. This caused some recent trouble for @edrumwri.

I am happy to take a crack at re-engineering this. My preference would be to get the caching code merged with the clunky-but-cloneable compatible ContinuousState to avoid massive API change but I could be talked out of that if it is urgent.

RussTedrake commented 6 years ago

not urgent. fine to get the caching in first! i just want to make sure that someone is voicing the core framework issues as we uncover them.

sherm1 commented 6 years ago

What do you think about this idea (from me, @edrumwri, @mitiguy):

Groups may be assigned certain properties:

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 variables, KE on config & velocity, regardless of whether those are being treated continuously at the moment.

Evan pointed out that we could actually allow an arbitrary tag to be associated with a state variable group, meaning that as-yet-to-be-written algorithms could look for those. We would reserve a few tags for the above entries.

I think the above could be done mostly backwards compatibly, though I'm not certain.

sherm1 commented 6 years ago

/cc @RussTedrake Would the above proposal address your problem dealing with ContinuousState?

RussTedrake commented 6 years ago

@sherm1 -- yes, I think this is the best solution that we have so far. Just a few minor questions.

I've thought less about the motivation behind the properties you mention. Is there a case where we have continuous state but NOT time derivatives?

A big question in my mind is whether we can lift the restriction that LeafSystemContext can only contain contiguous state. I can see why that might have seemed reasonable at one time, but I think it is very limiting (I currently cannot make an observer, a linear approximation, etc, of e.g. a diagram unless I vectorize its continuous state representation). I don't know where in LeafSystem that assumption got baked in, but I would love to see it removed during your cleanup pass?

sherm1 commented 6 years ago

Is there a case where we have continuous state but NOT time derivatives?

No -- can't treat the variables as continuous if we can't get a time derivative. But in this proposal "has_time_derivatives" would be a property that enables continuous treatment, while actually treating variables continuously would be determined by the analysis tool being applied.

OTOH, there are good use cases for "has_time_derivatives" but discrete updating -- time stepping & direct collocation could both be described that way since both can make use of derivatives in calculating their Δx.

A big question in my mind is whether we can lift the restriction that LeafSystemContext can only contain contiguous state.

I want to make sure I understand what you need, say for the Observer: Currently a leaf system's continuous state xc is a (contiguous) BasicVector, but q,v,z are Subvectors. In the proposal, a leaf system's xc would be a sequence of any number of independent BasicVector subgroups, which removes the requirement that the entire xc be contiguous. However, those groups still would have to be owned by the leaf system; they couldn't be pointers to BasicVectors owned elsewhere as is the case for a Diagram. Does that help you in putting together the Observer's state?

RussTedrake commented 6 years ago

Ah. I missed your idea that we actually zap the notion of continuous_state and discrete_state, and just have state. do second order (or configuration/velocity/general) labels have any value for states that do not have time derivatives?

Yes, your proposal addresses the observer problem. I intend to uphold the contract that the leaf system context own's it's own state, but just want to be able to have that be a (typed) copy of the state of another diagram. On construction, the observer would declare state by passing in the original system's state as a reference type (to be cloned).

sherm1 commented 6 years ago

do second order (or configuration/velocity/general) labels have any value for states that do not have time derivatives?

@edrumwri and I discussed this and agree that they do. The 2nd order/1st order pairing implies that the update formula for the 2nd order variables should involve its 1st order partner. Most likely this won't matter much because we ask the leaf systems to do their own discrete updates, and they are in a good position to know the proper update formulas. But if we wanted to make a generic solver for 2nd order discrete systems, we could do it by defining the update formula for the 2nd order variables to be the current values of their 1st order mates. See for example this doc, section 8.1 on page 35.

RussTedrake commented 3 years ago

I find myself revisiting this question / idea, after seeing the hoops that we are jumping through in #15528 to make the continuous state data reveal its true BasicVector self (in that case, for performance reasons). I wish that the efforts made in MultibodyTree could be applied more broadly to address this issue.

RussTedrake commented 3 years ago

Q: Why not have BasicVector<T> change its member variable to an Eigen::Map, with the option to own, or not own, the underlying vector data? Then we could have SuperVector<T> isa BasicVector<T>, thus allowing DiagramContinuousState (in additional to ContinuousState) to use BasicVector?

RussTedrake commented 3 years ago

This quick spike test suggests that we might have to change some return types, unfortunately, but it still seems doable.

jwnimmer-tri commented 3 years ago

Quick summary from the meeting this morning -- Eigen::Map is not a sufficiently general enough scatter-gather to allow for viewing a diagram's overall qvz vector as individul leaf qvz vectors, either with Diagram owning and Leaf as sub-views, or Leaf as owning and Diagram as a composite view. The various qvz offsets cannot be expressed with Eigen strides. (Think of a Map as a Matrix slice.)

My bid for "next idea to investigate" (possibly related to #9171) would be to do a benchmark and check if copying on demand is a sufficiently performant way to implement the slicing / compositing, compared to the offset-indexing math that we have currently.

The other idea that came up was to make the DoCalcTimeDerivatives callback to the user have friendlier arguments -- either a LeafContext instead of a Context, or an Eigen::VectorXd* result instead of ContinuousState<T>* derivatives, or both.