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

code review
1

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

2

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 readme.md :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:

2 Likes
3

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,
Chris

4

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.