RobotLocomotion / drake

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

Provide System-level mechanism for Allocating inputs #5343

Closed RussTedrake closed 7 years ago

RussTedrake commented 7 years ago

For motivation, see: https://github.com/RobotLocomotion/drake/blob/master/drake/systems/trajectory_optimization/direct_collocation.cc#L28

This toolchain needs to be able to create inputs to supply to the provided System*, and currently simply creates BasicVector's because it doesn't know any better. This is insufficient for systems that expect typed input, e.g., https://github.com/RobotLocomotion/drake/blob/master/drake/automotive/simple_car.cc#L109

RussTedrake commented 7 years ago

I would like this to live at the System level. I feel that there are two natural choices: 1) define

virtual std::unique_ptr<SystemOutput<T>> AllocateInput(
      const Context<T>& context) const = 0;

and answer the question about whether we re-use the SystemOutput type or actually create a new SystemInput type (that is essentially identical). Possibly typedef SystemOutput SystemInput could make the code more palatable.

2) provide

  virtual std::unique_ptr<BasicVector<T>> AllocateInputVector(
      const InputPortDescriptor<T>& descriptor) const {
    return std::make_unique<BasicVector<T>>(descriptor.size());
  }

  virtual std::unique_ptr<AbstractValue> AllocateInputAbstract(
      const InputPortDescriptor<T>& descriptor) const {
    DRAKE_ABORT_MSG("A concrete leaf system with abstract input ports must "
                    "override AllocateInputAbstract.");
  }

at the System level. The only thing that I don't like about that is the lack of parallelism with the current System/LeafSystem design. AllocateOutputVector is currently only defined for LeafSystem. But apart from the lack of parallelism, I actually think it's the right thing to do, so will PR that first. @david-german-tri , @sherm1 -- would appreciate your comments.

RussTedrake commented 7 years ago

WIP for consideration: https://github.com/RussTedrake/drake/commit/e96090b1f9442ade650fef95466fffaa794b7a5c If people approve of this design, then we can add the unit tests.

jwnimmer-tri commented 7 years ago

Approach (2) seems like the right System API to me.

The only other big question to me is whether CreateDefaultContext should end up with freestanding correctly-typed input ports by default, or nullptr inputs like it does now. I am hard-pressed to come up with reasons why anyone who asks a System to make a Context would want null inputs in general; it seems much more reasonable to provide default freestanding inputs wired up, and then the caller can set_value them if they want something else (as with any other state, or params, etc).

The exception to that rule would be for Systems with optional input ports, where the behavior of the system changes when a port is disconnected versus all-zeros. In that case, perhaps AllocateVectorInput should be returning nullptr, though that doesn't meet the need for abstract-valued optional inputs. Perhaps we should dismiss inferred-by-null optional inputs anyway, and require System authors to explicitly ignore inputs rather than try-null use them.

RussTedrake commented 7 years ago

I think the reason to have nullptr inputs is because of the (most common) input case where we are wiring the output of another system into the input of the current system. Why incur the cost of allocating an input vector if it's about to be thrown away?

jwnimmer-tri commented 7 years ago

Perhaps. But my understanding is that CreateDefaultContext should be a rare call -- Context::Clone is what simulation or control loops should be using.

And in any case, Context-creation is always going to be relatively expensive (it will always hit the heap several times), so it doesn't seem like one more should make a meaningful difference.

jwnimmer-tri commented 7 years ago

Maybe the compromise is to have System::CreateDefaultContext leave the inputs null, but offer a System::AllocateFreestandingInputs(Context*) that allocates them into a passed-in context, using the System's allocators with freestanding input ports.

david-german-tri commented 7 years ago

Maybe the compromise is to have System::CreateDefaultContext leave the inputs null, but offer a System::AllocateFreestandingInputs(Context*) that allocates them into a passed-in context, using the System's allocators with freestanding input ports.

I like this, because I don't want to give up on optional input ports, because it separates shape from content, and because System authors that don't have a need to implement it experience no additional overhead.

david-german-tri commented 7 years ago

Also, in the motivating dircol example, why not require that appropriate inputs are already hooked up to the Context, and use system_->EvalVectorInput? Then it's general to both freestanding usage and within-a-Diagram usage.

sherm1 commented 7 years ago

Sorry I'm late to this party. Thoughts:

jwnimmer-tri commented 7 years ago

In other words, the counter-proposal for the motivating example is that the Context that is passed must be wired up to inputs somehow, and then internally we Eval and then Clone those input values to become freestanding ports in our internal Context?

sherm1 commented 7 years ago

Right, just as the motivating example is doing for time, state, and parameters:

context_->SetTimeStateAndParametersFrom(context);

could be

context_->SetTimeStateParametersAndInputsFrom(context);