[BUG] combination_index and n_channels_start are not updated within the loop

[zhijian0111]

Describe the bug

in _multirocket.py
_transform_multi(X, X1, parameters, parameters1, n_features_per_kernel, indices, seed)
Within the section handling first order difference, it seems that combination_index and n_channels_start are not updated within the loop

Steps/Code to reproduce the bug

No response

Expected results

combination_index += 1
n_channels_start = n_channels_end

Actual results

missing in the loop

Versions

No response

[TonyBagnall]

at first glance this does look like a bug, the update should happen at line 869 in the loop

       for dilation_index in range(n_dilations1):

[patrickzib]

The original MultiRocket implementation does it the same way:

https://github.com/ChangWeiTan/MultiRocket/blob/3ccaa4f8ed11769b904af885e17018c39c89a2ac/multirocket/multirocket_multivariate.py#L479

[zhijian0111]

The original MultiRocket does not update the variable for the first order, and in their paper, they also do not provide details about the channel information. However, it makes sense to update the channel selection, since they have the channel combination variable for the first-order differences.

[TonyBagnall]

I think the multivariate was always a bit of an add on. Further possible issue is the difference parameters are stored in parameter1

        if n_channels == 1:
            X = X.squeeze()
            self.parameter = self._fit_univariate(X)
            _X1 = np.diff(X, 1)
            self.parameter1 = self._fit_univariate(_X1)
        else:
            self.parameter = self._fit_multivariate(X)
            _X1 = np.diff(X, 1)
            self.parameter1 = self._fit_multivariate(_X1)

but we then ignore the

def _transform_multi(
    X, X1, parameters, parameters1, n_features_per_kernel, indices, seed
):
    n_cases, n_channels, n_timepoints = X.shape
    (
        n_channels_per_combination,
        channel_indices,
        dilations,
        n_features_per_dilation,
        biases,
    ) = parameters
    if seed is not None:
        np.random.seed(seed)

    _, _, dilations1, n_features_per_dilation1, biases1 = parameters1
    n_kernels = len(indices)
    n_dilations = len(dilations)
    n_dilations1 = len(dilations1)

I'll ask angus and chang wei, this code could do with some commenting. This is also in the original code

https://github.com/ChangWeiTan/MultiRocket/blob/3ccaa4f8ed11769b904af885e17018c39c89a2ac/multirocket/multirocket_multivariate.py#L479

def transform(X, X1, parameters, parameters1, n_features_per_kernel=4):
    num_examples, num_channels, input_length = X.shape

    num_channels_per_combination, channel_indices, dilations, num_features_per_dilation, biases = parameters
    _, _, dilations1, num_features_per_dilation1, biases1 = parameters1

[TonyBagnall]

@zhijian0111 thanks for spotting this, I've contacted the authors and will get back to you.

[angus924]

Great catch @zhijian0111.

Yes, I believe this is a bug: combination_index and n_channels_start should be updated in the loop as suggested.

As to reusing num_channels_per_combination and channel_indices for the first difference, I think this was more of a choice, rather than a bug. Doing it this way (per the original code), or using a different set of values (i.e., taken from parameters1) are both ok from my point of view. I doubt it would make a whole lot of difference either way.

CC @ChangWeiTan in case he has a view here.

Thanks very much.

[TonyBagnall]

thanks @angus924 I would prefer to keep it the same as the original on the second point to avoid divergence. I think the second change should be evaluated before inclusion to see if it makes a difference.