borglab / gtsam

GTSAM is a library of C++ classes that implement smoothing and mapping (SAM) in robotics and vision, using factor graphs and Bayes networks as the underlying computing paradigm rather than sparse matrices.
http://gtsam.org
Other
2.6k stars 764 forks source link

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

Open dllu opened 4 years ago

dllu commented 4 years ago

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:

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 commented 4 years ago

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.