RobotLocomotion / drake

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

Subsystems that poll named vector inputs fail when T = AutoDiffXd. #8921

Closed jadecastro closed 6 years ago

jadecastro commented 6 years ago

There seems to be a failure happening when a Diagram is set up in such a way that one subsystem polls an input which is a subclass of BasicVector, and when T = AutoDiffXd. Under such circumstances, a std::logic_error like the following is thrown:

System::EvalVectorInput(): expected value of type drake::automotive::DrivingCommand<drake::AutoDiffXd> for input port[0] but the actual type was drake::systems::BasicVector<drake::AutoDiffXd>. (Subsystem ::AutomotiveSimulator::idm_car_simple_car)

but no such error is thrown when T = double.

This error can be reproduced via bazel run //automotive:automotive_simulator_test after making these changes to automotive_simulator_test.cc. In this particular example, SimpleCar polls a DrivingCommand<T>, whose input is connected to the output of a Multiplexer, which is also of type DrivingCommand<T>.

An error is thrown when stepping the AutoDiffXd Simulator at line 487, but not when stepping the double Simulator at line 480. The current workaround is to configure SimpleCar with only BasicVector inputs (and reference the vector elements using DrivingCommandIndices), but authors shouldn't need to rely on that. Also, note that this seems to be particular to Diagrams, as we are already using SimpleCar<AutoDiffXd> elsewhere.

sherm1 commented 6 years ago

I'm looking at this @jadecastro. Looks like the multiplexer didn't preserve the concrete BasicVector type when transmogrified. Might be fallout from recent input port changes. Great error message though! :)

jwnimmer-tri commented 6 years ago

This is a bug in Multiplexer's transmogrification support, due to @jadecastro's e55007a1 (#7551), not a bug in the framework.

jwnimmer-tri commented 6 years ago

Specifically, in this code ...

template <typename T>
template <typename U>
Multiplexer<T>::Multiplexer(const Multiplexer<U>& other)
    : Multiplexer<T>(other.input_sizes_,
                     systems::BasicVector<T>(other.get_output_port(0).size())) {
}

... the hard-coded BasicVector fails to preserve the model vector's subtype.

jwnimmer-tri commented 6 years ago

My suggested fix would that Multiplexer should not try to offer subtyping, but rather just do the multiplexing. We could / should have a VectorCast system (or similar -- possibly with a gain term, or even just enhance the Gain block) that turns untyped input into typed output.

sherm1 commented 6 years ago

Yeah, I just found that too. Should be easy to fix if we can transmogrify a BasicVector.

jwnimmer-tri commented 6 years ago

I'm not sure that values should ever intrinsically transmogrify; see #5454 for discussion. Here, we don't need to transmogrify the value, we just need to remember the model type. I have a prototype of this in my https://github.com/jwnimmer-tri/drake/tree/framework-vectorsystem-symbolic work.

sherm1 commented 6 years ago

OK, I see that's easier said than done! Wouldn't be hard to add a limited set of VectorBase::ToAutoDiffXd()-like virtuals (with an update to the BasicVector generator) but general doubly-templatized transmogrification seems out of reach. @jwnimmer-tri do you think that's worth doing?

jwnimmer-tri commented 6 years ago

I don't think we should transmogrify values on a whim. As #5454 discusses, the difference between SetFromDouble and ToOtherScalar is important.

I think that for now, I'd say if one wants a system to output a custom vector subclass, then one should write a System that does so. That would solve the current use case and a bunch of related ones. It's not like the Multiplexer primitive is buying us a lot right now -- writing a DrivingCommandMux is an easy work-around, and would provide better documentation in the automotive_simulator (names for the acceleration_input_port and steering_angle_input_port, etc.) -- right now, the simulator brittlely hard-codes the DrivingCommand coordinate indices of 0 for steering angle and 1 for acceleration.

sherm1 commented 6 years ago

In that case I think the bug here is that the Multiplexer's documentation doesn't mention that it doesn't preserve the BasicVector subtype when ToAutoDiffXd() is called. @jadecastro do you want to fix the documentation?

jwnimmer-tri commented 6 years ago

In particular, the Multiplexer(const systems::BasicVector<T>& model_vector); constructor should disable scalar conversion entirely. See my recent changes to primitives/constant_vector_source.h (#8794) for an example.

jadecastro commented 6 years ago

I see. I will work on a fix to disable the scalar type conversion for the model_vector constructor, add the needed documentation, and add a new DrivingCommandMux to patch it up locally. Just sad we have to go the extra step to make an AutoDiff-Diagram work, though.