RobotLocomotion / drake

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

Should FixedInputPorts survive transmogrification? #5872

Open RussTedrake opened 7 years ago

RussTedrake commented 7 years ago

Currently they do not. The current workflow is (in pseudo-code):

auto autodiff_system = original_system->ToAutoDiffXd();
auto autodiff_context = autodiff_system->CreateDefaultContext();
autodiff_context->SetTimeStateAndParametersFrom(original_context);

as seen in, e.g., https://github.com/RobotLocomotion/drake/blob/master/drake/systems/primitives/linear_system.cc#L48

I'm starting to see non-trivial systems which naturally have FixedInput ports assigned deep in their bowels. For example, an automotive simulator with many vehicles driving around, some of which are wired up with FixedInput acceleration commands. These do not currently survive the transmogrification step -- in fact the resulting autodiff_systems output methods cannot be evaluated (because they have unwired inputs).

I see a few possibilities here:

System architects, what are your thoughts?

david-german-tri commented 7 years ago

require internal uses like this to use input systems explicitly (e.g. ConstantVectorSource). this will still require implementing ToAutoDiffXd() for BasicVector (or templating ConstantVectorSource on the Derived type), as discussed in the review of #5852.

I tentatively prefer this, the principle being: FixInputPort is a convenience for top-level Systems, and shouldn't be baked into a Diagram. We could enforce that at runtime: if there is no graph edge terminating at a given subsystem's input port, and yet that input port is not nullptr, diagram.h throws.

Also, this makes ConstantVectorSource seem like an extremely special case, which makes me prefer templating on the vector type to adding new vector APIs. (I balk at the phrasing "Derived type", because that sounds like CRTP, which it isn't.)

provide a method similar to SetTimeStateandParametersFrom() which transmogrifies the fixed inputs to the context. (or add it to that method, suitably renamed).

This is similar in spirit to System::FixInputPortsFrom. The differences are: it would descend recursively through Diagrams, and when descending through Diagrams it would only fix input ports that are not part of the Diagram graph. I could live with it, but I don't think it's a great payoff for the additional API bulk.

RussTedrake commented 7 years ago

I hadn't seen FixInputPortsFrom. That's very relevant. Thanks.

ConstantVectorSource and ConstantValueSource are special cases, for sure. I initially thought the same tools would be useful for any system using DeclareVectorInputPort, DeclareVectorOutputPort, etc.... but now I'm convinced that I was incorrect. I guess it would only happen when the type of the output, etc, are only know dynamically, which I admit is more rare. Could it happen for systems that construct their output ports dynamically (e.g. multibody?)?

If we need to implement DoToAutoDiffXd() for systems with input/output types that are only known dynamically, then I think that implementing BasicVector::ToAutoDiffXd() is probably the right way.

david-german-tri commented 7 years ago

I guess it would only happen when the type of the output, etc, are only know dynamically, which I admit is more rare. Could it happen for systems that construct their output ports dynamically (e.g. multibody?)?

If we need to implement DoToAutoDiffXd() for systems with input/output types that are only known dynamically, then I think that implementing BasicVector::ToAutoDiffXd() is probably the right way.

Yeah, I agree, the question is how many of those are there. I have yet to think of an example besides ConstantFooSource - which is why I tentatively favor solving it on the System side, instead of adding boilerplate to every vector that goes into a ConstantVectorSource.

sherm1 commented 7 years ago

@RussTedrake, there was a recent review thread on this topic triggered by @edrumwri's use of FixInputPortsFrom() in creating autodiff Jacobians for the implicit integrator, prompting a Slack discussion and issue #5813, PTAL.

I think that fixed input ports should be transmogrified along with everything else, ideally in a single Context method that would just be an extension to Context::SetTimeStateAndParametersFrom(). But for now having to make an extra call to System::FixInputPortsFrom() is adequate, if clumsy.

BTW I don't have any philosophical objection to fixed input ports. For a simulation to be performed on a System, all inputs must be resolved except for time, which is the only input a Simulator can supply. I don't see a fundamental reason why the only possible value source for an input is someone else's output. I am happy with input ports that have default values, or are resolved internally to parameters, which is essentially what we are doing with fixed input ports. Ideally IMO we would literally use the existing Parameter mechanism as an option for resolving input ports; I don't think we need a separate way to provide numerical or abstract values.

david-german-tri commented 7 years ago

@RussTedrake, there was a recent review thread on this topic triggered by @edrumwri's use of FixInputPortsFrom() in creating autodiff Jacobians for the implicit integrator, prompting a Slack discussion and issue #5813, PTAL.

That was not exactly the same topic - #5575 needed to transmogrify and fix input ports only for the top-level System under simulation, so FixInputPortsFrom sufficed. Alejandro found it offensive for some reason I didn't understand, and proposed an improvement that I also didn't understand, but no one disagreed that FixInputPortsFrom does the job.

This issue contemplates transmogrifying fixed input ports that are buried deep within a Diagram, which FixInputPortsFrom won't do as written. It would be possible to generalize, as discussed above, but I'd sort of prefer to say "don't do that, use ConstantFooSource instead", also as discussed above.

BTW I don't have any philosophical objection to fixed input ports.

I don't think anyone does?

Ideally IMO we would literally use the existing Parameter mechanism as an option for resolving input ports; I don't think we need a separate way to provide numerical or abstract values.

I don't understand this suggestion. The existing mechanism requires a System to allocate its Parameters, which would require a System to know the leaf type of its inputs, which is not always so (e.g. Adder).

sherm1 commented 7 years ago

This issue contemplates transmogrifying fixed input ports that are buried deep within a Diagram

Oh, thanks @david-german-tri I missed that twist. Then I do think the best solution is to extend the Context transmogrifier to include the fixed input values as well.

Re use of Parameters for fixed input port values: I see your point. It seems a shame we need a separate mechanism though just to accommodate last-minute values. Parameters provide a great mechanism for supplying arbitrary values in a Context that don't change with time. We can autodiff with respect to them, and they are already copied and transmogrified successfully. Those are the attributes we want for input port values too; it would be nice to have a single mechanism IMO.

RussTedrake commented 7 years ago

maybe we're on the same page now about the problem (transmogrification of fixed input ports buried deep in a diagram). but so far it seems we have different opinions about the solution. we have two proposals: 1) extend context transmogrification (actually Construct then SetFrom...) to support internal fixed input ports or [ @sherm1 favors ] 2) require users to do ConstantVectorSource inside a diagram [ @david-german-tri favors ]

Both 1 and 2 also require a decision about whether we add an NVI implementation for ToAutoDiffXd to BasicVector or template ConstantVectorSource/FixedInputYada.

sherm1 commented 7 years ago

I have to confess that I don't feel strongly about option 1. If it isn't an unreasonable user burden to satisfy all the internal inputs with option 2's ConstantXXXSources, then I am fine with requiring that. We could relax the requirement later if necessary. Option 2 requires upgrading ConstantVectorSource to preserve the vector type and be autodiffable, but that seems like a good thing to do anyway.

jwnimmer-tri commented 7 years ago

I don't think how "deep" the fixed inputs are buried is a relevant concern. Even at the outermost layer this can still bite us, and automotive's current fixed input are at the outermost diagram already (there's only one diagram, no nesting).

I think either of the OP's two proposals are reasonable.

My rough impression is that SetTimeStateandParametersFrom is supposed to be a "make this Context be like that other Context". So at a minimum, if it sees a Fixed input and isn't going to deal with it, it should fail immediately.

Given that, it seems like repairing SetTimeStateandParametersFrom to deal with Fixed inputs could be the most elegant solution. Depends on how the implementation turns out, probably.

I think it's also worth noting that the code snippet in the OP shouldn't be anything users have to paste all over the place, either. It should be rolled up into a transmogrification helper function that code can just call to get a new system and context, given an old system and context.

Separately, we should probably make ConstantVectorSource (or really, most of the primitives) support transmogrification; and FYI we can do that for ConstantVectorSource without adding any extra class-template argument, just some constructor / factory sugar.

RussTedrake commented 7 years ago

@jwnimmer-tri - possibly relevant to your current design discussion.

jwnimmer-tri commented 7 years ago

I just re-read the whole thread. I think my most recent comment above still stands.

The one new thing we could do is to make ConstantVectorSource support transmogrification (while preserving the BasicVector subclass) after #6660 and #6589 (and its follow-up for LeafSystem) land.

sherm1 commented 4 years ago

A few years on, this seems much clearer: #12674.