Repository navigation
[ENH] Add multirocket multivariate cython implementation - #7
Conversation
| def multirocket_transform( | ||
| X, | ||
| parameters, | ||
| n_features_per_kernel=4, |
There was a problem hiding this comment.
It's more of a side note after testing aaround (@fkiraly):
The output array is sized from this parameter, and the differenced block starts at half that width (.pyx L272-276):
num_total_features = (num_features + num_features1) * num_features_per_kernel
num_features_per_transform = num_total_features >> 1
_one ignores the parameter and always writes 4 features per kernel, one block of num_features columns each (.pyx L176-183). With k = 2, the array is 4 nf wide, and the differenced pass writes up to 6 nf. Each row writes into the next one, and the last row writes past the end of the array. Bounds checking is off, so nothing stops it. With k = 8, the array is
16 nf wide but only 8 nf columns are written.
sktime has the same code (_multirocket_multi_numba.py L703-707: the same allocation, the same features.shape[1] / 2 offset, 4 writes per kernel):
X = rng.normal(size=(64, 3, 500))
# this PR
p = multirocket_fit(X, num_kernels=10_000, random_state=0)
for _ in range(50):
multirocket_transform(X, p, n_features_per_kernel=2)
# Out: Segmentation fault (exit 139)
# sktime
t = MultiRocketMultivariate(num_kernels=10_000, n_features_per_kernel=2, random_state=0).fit(X)
for _ in range(50):
t.transform(X)
# Out: Segmentation fault (exit 139)There was a problem hiding this comment.
Yeah that's a bit silly, this shouldn't be a parametrizable value at all... In the original implem it's set to 4 within the __init__ block but it's not an arg of the method. There are always 4 features per kernel.
|
LGTM @fkiraly Let me know if we can merge. |
|
quick question, is this stacked on #6? |
fkiraly
left a comment
There was a problem hiding this comment.
High-level review: can we leave setup.py to accept explicit modules? There might be other modules than just rocket ones, e.g., another contributor could add cython code pertinent for another sktime module.
No, both #6 and #7 are rebased on main. I'll rebase it if we merge #6 first. Order should not matter here.
Do you mean something like this? _MODULES = [
"sktime_cython.transformations.rocket._minirocket_multivariate_cython",
"sktime_cython.transformations.rocket._multirocket_multivariate_cython",
]Cause otherwise Cython accepts something like: extensions = [
Extension(
"*",
["sktime_cython/**/*.pyx"],
include_dirs=[np.get_include()],
define_macros=[("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION")],
extra_compile_args=_fast,
)
]And it should do it automatically. |
…without casting (#6) <!-- Thanks for contributing a pull request! Please ensure you have taken a look at our contribution guide: https://github.com/alan-turing-institute/sktime/blob/main/CONTRIBUTING.md --> #### Reference Issues/PRs None. #### What does this implement/fix? Explain your changes. Adds univariate MiniRocket support to the existing compute API: - `rocket_fit` / `rocket_transform` now accept a 2D `(n_instances, n_timepoints)` panel in addition to 3D. 2D input is the single-channel case `(n_instances, 1, n_timepoints)`. - No separate univariate functions: on one channel, the multivariate algorithm always selects channel 0, and bias fitting re-seeds before drawing instance indices. The result is bit-identical to a dedicated univariate path. The same holds in sktime: `MiniRocket` and `MiniRocketMultivariate` produce identical features on single-channel input. - Empty panels and inputs that are not 2D or 3D are rejected with a `ValueError`. - The Cython kernels are fused over float32/float64 (as in #7), so float64 input is no longer copied to float32. Values are rounded to float32 as they are read, so results are bit-identical to casting first and match sktime MiniRocket, which works in float32. Computing natively in float64 was 1.5–1.9× slower in `transform` and would drift from sktime. #### Does your contribution introduce a new dependency? If yes, which one? No. #### What should a reviewer concentrate their feedback on? - [ ] Whether accepting 2D input in `rocket_fit` / `rocket_transform` is the preferred way to expose the univariate case. - [ ] Computing in float32 for float64 input (matching sktime MiniRocket) rather than natively in float64 as in #7. #### Any other comments? 2D output matches sktime `MiniRocket` across multiple seeds, kernel counts and dilation limits. 2D and `(n, 1, d)` input give identical parameters and features, including the threaded path. float64 and integer input give parameters and features identical to casting to float32 first. #### PR checklist ##### For all contributions - [x] I have added myself to the [list of contributors](https://github.com/alan-turing-institute/sktime/blob/main/.all-contributorsrc). - [x] Optionally, I have updated sktime [CODEOWNERS](https://github.com/alan-turing-institute/sktime/blob/main/CODEOWNERS) to receive notifications about future changes to these files. - [x] I have added unit tests and made sure they pass locally. ##### For new estimators - [x] I have added the estimator to the README estimator table. <!-- Thanks for contributing! --> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
729f866 to
cd9d1bf
Compare
b069e26 to
8db1161
Compare
Reference Issues/PRs
See also sktime/sktime#11292 regarding the
original_implementationflag.What does this implement/fix? Explain your changes.
This PR adds a cython estimator for multirocket (multivariate). In doing so it tries to reuse identical code from the minirocket implementation by @sssilvar (fitting the dilations, and computing the per-combination quantiles, which I've rewritten in a vectorized fashion, along with a non-regression test).
This cython implem has the advantage of accepting either one of float32/float64 inputs, without enforcing a cast, relying on the cython floating dtype.
Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
Probably the conjunction of this PR and the one related to the multirocket bug in the current sktime implementation. I'm not quite sure whether merging this PR before the sktime one previously referenced is the correct way to go; upon merge of this PR, we'd also need to wrap the estimator in the main repo, as was done for _minirocket_cython.
Any other comments?
PR checklist
For all contributions
For new estimators