Feedback: Dynamic Time Warping implementation in Rust (Python Bindings)

Hi All,
I've implemented dynamic time warping in Rust with Python bindings. This is my first Rust project so some input from more experienced folk would be appreciated.

repo here

Hey, I remember you from the pyo3 gitter :wave:

First of all, you should send a pull request to list your project in pyo3's :wink:

I don't really understand much about the problem space, but this sprung out to me:

You pass around the "distance mode" parameter, whose type is a String, which represents how distance is measured. This is nicknamed a "stringly typed system" (heh) and will make your program brittle and a pain to debug when it gets bigger.

For example, you have this code:

if distance_mode.eq("euclidean") {
      f64::sqrt(dtw[[n - 1, m - 1]])
} else {
      dtw[[n - 1, m - 1]]

This is subtly incorrect: when someone tries using a mode other than Euclidean or Manhattan this function will assume Manhattan distance. With an enum you can leverage Rust's type system and make this harder to get wrong:

match distance_mode{
    MyDistanceModeEnum::Euclidean =>   f64::sqrt(dtw[[n - 1, m - 1]]),
    MyDistanceModeEnum::Manhattan=>  dtw[[n - 1, m - 1]],

If you define these modes as an enum and implement FromPyObject for it, then your python function can look like this:

#[pyfn(m, "dtw")]
    fn dtw_py(
        s: PyReadonlyArray1<'_, f64>,
        t: PyReadonlyArray1<'_, f64>,
        window: i32,
        distance_mode: MyDistanceModeEnum,
    ) -> f64 { /* ... */ }

This will also do away with the error handling in the select_distance function.

A minor nit regarding the readme: I don't know what "Dynamic Time Warping" and "functional brain regions" mean. As such the readme didn't make much sense until I looked up the terms. A paragraph summarizing the problem space would be nice to have for laymen like myself :flushed:


Hi @mejrs ,
Thanks for your help the other day!

Really appreciate you taking the time to go through the code and the suggestions look great, I will be implementing them today!

Many Thanks,

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.