-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH] Merge MultiRocket and MultiRocketMultivariate #1711
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Thank you for contributing to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good, just got a question about methods (such as fit biases) that are still split between uni and multivariate, what is the reason exactly ?
Couldn't you just use the same method and affect channel_indices
to channel 0 and/or num_channels_per_combination=1
for all kernels in the univariate case ?
you maybe could, but I want to do that on a separate PR for this one, I am not convinced as to how the multivariate is handled. See my comment at the top of the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering Tony's comment on rationalising common structures between rockets transformers later, I'd say that this PR has served its goal to put base structure in place.
I'll make a rocket related issue |
merges the two classes MultiRocket and MultiRocketMultivariate. Part of #1699 fixes #1713
Unlike minirocket, I will keep the transform functions within multirocket separate and raise an issue about how the multivariate version is implemented. I think