Symbolic + VectorSystem is boilerplatey and fails silently

[ggould-tri]

Given that VectorSystem is the easiest entry-point for new Drake users, and given that most Drake systems can and should support Symbolic, the boilerplate described in the docs to make VectorSystem usable with Symbolic (and sparsity) is a hard hill to climb.

What makes it worse is that if you make a mistake (eg, leaving out the SystemTypeTag bit) your code will go silently uncompiled and the error you eventually get will not point you in the right direction at all.

Jeremy Nimmer asserts that CRTP could be used to avoid this.

More broadly, having written seven symbolic VectorSystems I now find I have to make a whole lot of changes across all seven files -- a sure sign of boilerplate disease -- and similarity-tester indicates that >90% similarity between trivial passthrough systems. It really ought to be possible to pass a state derivatives lambda (or something similar) to a factory method as a one-stop shop.

[ggould-tri]

(@jwnimmer-tri )

[jwnimmer-tri]

The CTRP idea was not very good, as it turns out. (You still need to write a transmogrification constructor, to preserve the most specific subtype.)

Therefore, I think it would be better if this API always used the same concrete type (SymbolicVectorSystem<T>) but allowed you to specify all of the context sizes and calc functions (output, derivatives, step, etc.) as constructor arguments.

@ggould-tri @RussTedrake et. al., what do you think about the following as a de-boilerplated user API?

namespace {

// Makes a System whose output is a constant 1-element vector.
auto MakeSource(double value) {
  return std::make_unique<SymbolicVectorSystem<double>>(
      OutputSize(1),
      [=](const auto& context, auto& output) {
        output[0] = value;
      });
}

// Makes a System whose output is a constant factor times the input.
auto MakeGain(int size, double factor) {
  return std::make_unique<SymbolicVectorSystem<double>>(
      InputSize(size),
      OutputSize(size),
      [=](const auto& context, const auto& input, auto& output) {
        output = input * factor;
      });
}

// Makes a System whose input is acceleration and output is position.
auto MakeSecondOrderSystem() {
  return std::make_unique<SymbolicVectorSystem<double>>(
      InputSize(1),
      OutputSize(1),
      [](const auto& context, const auto& input, const auto& state, auto& output) {
        output[0] = state[0];
      },
      ContinuousStateSize(2),
      [](const auto& context, const auto& input, const auto& state, auto& derivatives) {
        derivatives[1] = input[0];
        derivatives[0] = state[1];
      });
}

void Build() {
  DiagramBuilder<double> builder;
  auto* source = builder.AddSystem(MakeSource(5.));
  auto* gain = builder.AddSystem(MakeGain(1, 2.));
  auto* plant = builder.AddSystem(MakeSecondOrderSystem());
  builder.Cascade(*source, *gain);
  builder.Cascade(*gain, *plant);
}

}  // namespace

Things to note:

1. Factory methods are the abstraction mechanic, not VectorSystem subclassing.
2. You declare your Context shape in TypeSafeIndex-tagged constructor arguments. a. If you don't list InputSize, you have no input port, and the callbacks don't have input as a parameter. b. If you don't list ContinuousStateSize, you have no continuous state, and the callbacks don't have state as a parameter. (You can only list one of ContinuousStateSize or DiscreteStateSize.) c. As a common-case optimization, we could probably change this to omit the context argument in the callbacks unless you first say a magic-knock tag like CallbacksUseContext{} as one of the first constructor arguments.
3. The generic lambdas are (during construction) immediately transmogrified into every function signature that we need for them (double, AutoDiff, symbolic), and shared among all of the transmogrified flavors of the Systems that employ them.
4. By disallowing subclassing, we have neatly avoided the problem of users trying to bury undeclared state in member fields, especially T-dependent state (you essentially can't say T in the lambda syntax (until C++20 anyway)).
5. I suspect that we could refine this approach to use either inferred (via the callback signatures) or declared (in the constructor arguments) vector_gen-based types, so that you could use named accessors like input.tau in the lambda equations.

Do we like this general approach?

---

I've pushed an unfinished draft of the implementation to: https://github.com/jwnimmer-tri/drake/tree/framework-vectorsystem-symbolic.

[RussTedrake]

The use case i would want to optimize is that a (potentially novice) user has a vector differential equation and they want to throw it into the systems framework. Like the simple_*_system examples. I don’t know if you’d plan enough Make boilerplates to cover them? Or would we be subjecting them to the extra complexity of the new templated types?

[jwnimmer-tri]

The Make functions were just examples the user could write. The framework API is the SymbolicVectorSystem constructor, which would offer overloads for whether or not there is input, continuous state, discrete state, possible even parameters, etc.

Right now in //examples, the continuous time example looks like

class SimpleContinuousTimeSystem : public drake::systems::VectorSystem<double> {
// ... 27 lines of overrides and stuff, _without symbolic support_ ...
};

int main() {
  // Create the simple system.
  SimpleContinuousTimeSystem system;

  // Create the simulator.
  drake::systems::Simulator<double> simulator(system);
...

My proposal could look something like (this has a few innovations vs my earlier post):

// (The class SimpleContinuousTimeSystem disappears; not needed here.)

int main() {
  SymbolicVectorSystem system(
      ContinuousStateSize(1),
      [](const auto& input, const auto& state, auto& derivatives) {
        using std::pow;
        derivatives(0) = -state(0) + pow(state(0), 3.0);
      },
      OutputIsState());

  // Create the simulator.
  drake::systems::Simulator<double> simulator(system);
...

[RussTedrake]

Got it. This is awesome, and should definitely exist. But perhaps it’s not a complete replacement? For instance, what if i want to make a VectorSystem with methods for which i don’t have symbolic support?

[jwnimmer-tri]

If you say SymbolicVectorSystem we would require that it work symbolically. We could offer SimpleVectorSystem variant that did only double (and probably AutoDiff?).

[RussTedrake]

Agreed. Without having paged in all of the details, I think it’s reasonable to have both?

[jwnimmer-tri]

Agreed.

I consider the above a spike test to see if we like the general flavor. Next step would be to work out exactly what names we use, and features we offer / require, so that a whole suite of APIs and names are available, consistent, and sensible.

[ggould-tri]

I really like the flavor of this -- it makes the easy things easy. If it can be made to work with named vectors -- even better! This is a real improvement, and might kill a great many lines of code from my branch.

[jwnimmer-tri]

I have a syntax that I like now:

I also have a few more examples in https://github.com/jwnimmer-tri/drake/blob/framework-vectorsystem-symbolic/systems/framework/test/symbolic_vector_system_test.cc (with more coming).

[sherm1]

Sweet!

One suggestion: name the auto dummy argument something other than system. I thought it was a Drake System until I read on. Maybe system_data or just data or args?

[RussTedrake]

Looks beautiful. I really like the fact that you can omit any parameters. But I would still put parameter_size last in the list (for style, and for those of us that don't read the docs and assume you have to pass all parameters you want until the last).

[SeanCurtis-TRI]

Very nice -- you do have a typo:

At most one kind one of either...

Also, question: the struct's time field is also T?

[sherm1]

Not clear from the constructor under what conditions the calc_next_state method is invoked. Is that left to the generic event system or simplified to a periodic update with period specified elsewhere?

[jwnimmer-tri]

One suggestion: name the auto dummy argument something other than system. I thought it was a Drake System until I read on. Maybe system_data or just data or args?

I also struggled with this. Initially I had context, which made a lot of sense, except that the argument wasn't truly a Context.

I liked system because then when read without thinking about it too much, you were clearly specifying the system's output, dervatives, etc., as you would in the set of equations in a book.

I plan to keep brainstorming on this.

Not clear from the constructor under what conditions the calc_next_state method is invoked. Is that left to the generic event system or simplified to a periodic update with period specified elsewhere?

Ah, indeed. A symptom of me not writing out those test cases yet. In VectorSystem we let the user declare the update in their constructor. Probably here I should add another constructor argument so its declared up-front. I'll see how that looks.

Also, question: the struct's time field is also T?

https://github.com/RobotLocomotion/drake/blob/3a1ac00df54fef6fdf03245cc40a9c261c6e6d98/systems/framework/context.h#L49

[jwnimmer-tri]

Recent updates to the feature branch:

One suggestion: name the auto dummy argument something other than system. I thought it was a Drake System until I read on. Maybe system_data or just data or args?

Done. I went with arg for now.

I would still put parameter_size last in the list...

Done.

... typo: At most one kind one of either...

Done.

Not clear from the constructor under what conditions the calc_next_state method is invoked. Is that left to the generic event system or simplified to a periodic update with period specified elsewhere?

Done. The period is required, the offset is optional (defaults to zero).

GTEST_TEST(SymbolicVectorSystemTest, SimpleDiscreteSystem) {
  // Simple Discrete Time System: x[n+1] = x[n]^3
  const SymbolicVectorSystem dut(
      DiscreteStateSize(1), DiscreteUpdatePeriod{1.0}, [](auto& arg) {
        using std::pow;
        arg.next_state[0] = pow(arg.state[0], 3.);
      },
      OutputSize(1), [](auto& arg) {
        arg.output = arg.state;
      });

[EricCousineau-TRI]

Would it be possible to lower the priority of this issue, or close it if it's already been resolved?

[jwnimmer-tri]

https://github.com/RobotLocomotion/drake/issues/20989#issuecomment-2002129534