RobotLocomotion / drake

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

Remove support for externally-provided scalar types #12814

Open jwnimmer-tri opened 4 years ago

jwnimmer-tri commented 4 years ago

This issue proposes to remove support for downstream-defined scalar types.

Many classes in Drake are templated on the type of numerical Scalar used for computation (e.g., double, AutoDiffXd, symbolic::Expression), usually denoted by template argument <T>.

We have https://github.com/RobotLocomotion/drake/blob/master/common/default_scalars.h which defines the typical scalars used within Drake, and provides macros to declare and define template instantiations across those types.

Some code in Drake (e.g., multibody, primitives) places most method definitions into cc files, which means that even though the class is templated, the permitted values for <T> are closed off to be only the default scalars and nothing else. Other code in Drake (framework) places method definitions in header files so that the set of valid <T> is open -- any real-number-like type can be used.

I propose that we switch all code in Drake to use the former pattern -- only support the default scalars.

This will allow us to move most method definitions to cc files instead of headers. (Code where inlining is important for performance would not move, nor would code that is templated on user-provided types such as MySystem or MyBasicVector.) That should substantially improve compile times, both inside Drake and for downstream users.

If we find good cause for scalars beyond the current defaults (e.g., #12019), we can add them to default_scalars.

If any Drake users need new scalars, please post below with details.


Edit: Added task checklist:

edrumwri commented 4 years ago

+1 on this- I would definitely trade a little bit of inflexibility in adding new scalar types for much faster compilation times.

RussTedrake commented 4 years ago

The two highest potential scalar types not currently in the default scalars, in my mind, are probably:

I still might trade compile time for features, but just want to think it through.

jwnimmer-tri commented 4 years ago

The challenge is that its difficult to incrementaly add a new scalar for just one specific use case to only the places its needed. Even leaf_system uses 149 source files in Drake (of course not all of them are templated on scalars), or //systems/primitives needs 261 files in total. Trying to add the new scalar one by one to all of the dependencies of some smaller example will end up death by a thousand cuts. For the core libraries at least, an agreed-upon common set of scalars seems like the correct trade-off. Code can opt-out of autodiff or symbolic or some-new-scalar like we do now, in cases where it doesn't make sense.

RussTedrake commented 4 years ago

In this case, it sounds like we effectively do not currently support externally-provided scalar types and having nothing to lose. SGTM.