FixedLagSmoother should use std::chrono instead of double for managing timestamps

[dllu]

Feature

FixedLagSmoother should use std::chrono types, or be templated to accept any type, for managing timestamps instead of double.

Motivation

Currently FixedLagSmoother has hardcoded the type for timestamps as double. However, this is not strictly correct, as it leads to:

• Floating point precision loss if one is trying to run a SLAM over a long period of time
• Confusion between units of timestamp, is it nanoseconds? seconds?
• Conflation between durations and absolute timestamps

The STL #include <chrono> provides templated types using either integer or floating point backend as zero-cost abstractions over using primitive integer or floating point types. It provides the notions of time_point and duration. In addition it becomes easy to use it with real system time, e.g. std::chrono::steady_clock::now().

Paying close attention to timestamp types may also bugs, as it disincentivises people from writing code that looks like

for(double time = deltaT; time <= 3.0; time += deltaT) {

which may iterate an unexpected number of types for some choices of deltaT.

Pitch

I skimmed through the code of FixedLagSmoother and its derived classes and it seems straightforward to replace double with another type.

For example, instead of double, we can have:

template<class ts_rep = double, class ts_period = std::ratio<1>>
class FixedLagSmoother {
    using duration_t = std::chrono::duration<ts_rep, ts_period>;
    using time_point_t = std::chrono::time_point<ts_rep, ts_period>;
    typedef std::map<Key, time_point_t> KeyTimestampMap;
};

Alternatives

Instead of using std::chrono we could also just template to let the user supply any type, so that users are free to decide whether they want to use std::chrono or not, e.g.

template <class ts>
class FixedLagSmoother {
    typedef std::map<Key, ts> KeyTimestampMap;
};

// later...
FixedLagSmoother<std::chrono::nanoseconds> nanosecond_smoother;
FixedLagSmoother<double> double_smoother;
// etc

Additional context

The ISO C++ Core Guidelines provides an example of an interface blink_led that takes a timestamp as an input.

[dellaert]

Nice.

I like the template solution, as it allows for a default template argument that we can set to double for backwards compatibility. Note there is the reverse map TimestampKeyMap that needs to be supported, too.

I'd love it if you'd put in a PR for this :-) if so please use TimeStamp as the keyword argument, and we should switch to using instead of typedef.