Repository navigation
Conversation
There was a problem hiding this comment.
I found several issues:
- [P1] Validate channel counts before entering the unchecked Cython loop
In _dtw_cython.pyx, _local_cost loops over x.shape[0] and reads y[k, j] with bounds checking disabled. If x has more channels than y, this reads beyond the memory allocated for y. A reproduction with shapes (2, 4) and (1, 4) returned the incorrect value 4.0 instead of raising an error. Both public entry points should require _x.shape[0] == _y.shape[0].
- [P1] Validate the bounding matrix shape
A custom bounding matrix is accepted without ensuring its shape is (m1, m2). The Cython kernels then access bm_mask[i, j] with bounds checking disabled, so an undersized matrix causes out-of-bounds reads and undefined behavior. This also affects automatically generated Itakura bounds for unequal-length series: _itakura_parallelogram creates (y_size, x_size), while the kernels require (x_size, y_size).
- [P1] Do not combine the infinity-based recurrence with incompatible fast-math assumptions
The extension uses INFINITY to initialize and block dynamic-programming cells, but setup.py compiles it with -ffast-math. Clang reports multiple use of infinity via a macro is undefined behavior warnings while building this PR. Precomputing the bounding mask avoids finite checks in the loop, but it does not protect the infinity values used by the recurrence. The DTW extension should be compiled without incompatible finite-math assumptions, or use a finite sentinel with overflow-safe handling.
- [P2] Expand legacy equivalence coverage beyond equal-length scalar distances
The current test_cython_matches_numba is a useful start, but it only exercises equal-length inputs and dtw_distance. Please mirror the Rocket regression approach across unequal lengths in both orientations, univariate and multivariate inputs, all bounding modes, custom masks, and dtw_cost_matrix. Add explicit validation tests for mismatched channel counts and incorrectly shaped bounding matrices. At least one regular CI job should install the dev extra so these comparisons run instead of being skipped; wheel tests can continue to skip them.
|
Thanks for the review! |
Reference Issues/PRs
Part of #4
What does this implement/fix? Explain your changes.
This PR implements ahead-of-time (AOT) compiled Cython dynamic time warping (DTW) kernels (
dtw_distanceanddtw_cost_matrix), providing a zero-warmup, high-performance C-extension alternative tosktime's Numba implementation.Key Changes
Cython DTW Kernels (
dtw_distance&dtw_cost_matrix)double[:, ::1]) withnogil.dtw_distancecomputes the final DTW cost via a rolling two-row buffer, reducing memory overhead toPrecomputed Vectorized Masking (
unsigned char[:, ::1])uint8mask (np.isfinite(bounding_matrix)).-O2/-O3/-ffast-math(e.g., Clang on macOS), providing ultra-fastnogilloops.Benchmark Results
Below is the end-to-end performance comparison between
sktime's Numba implementation (sktime.dists_kernels._numba_distances.dtw_distance) and the new ahead-of-time compiled Cython extension.Times in microseconds (µs). Benchmark measured across 500 calls with pre-allocated bounding matrices.
d,m)dtw_distance(µs)dtw_distance(µs)cost_matrix(µs)d=1, m=50d=1, m=200d=3, m=200d=5, m=500Key Performance
d=5, m=500), distance calculation finishes in < 1 ms (930 µs).