mlgig / mrsqm

GNU General Public License v3.0
28 stars 8 forks source link

should we add the `firstdiff` parameter in `sktime` too? #13

Open fkiraly opened 1 year ago

fkiraly commented 1 year ago

@lnthach, quick question: should we add the firstdiff parameter in sktime too?

You could make a PR to change the interface class, since changes in parameters do not propagate through the interface.

There's also the issue that the firstdiff parameter is only availale from 0.0.4 on, so we need to take care that users with lower versions don't suddenly error out. (only relevant if we change the interface in sktime)

Also, quick question: not sure whether there is a need to add this in mrsqm - you can easily get the same logic from the pipeline

from sktime.classification.shapelet_based import MrSQM
from sktime.transformations.series.difference import Differencer

sqm = MrSQM(some_params)
first_diff = Differencer()

sqm_with_first_diff = first_diff * sqm

:-)

Of course you decide which parameterizations you want to ship with your estimator, e.g., for convenience, or as a scientific implementation reference to a paper. Just pointing out that users of pipelines can get this (and similar) functionality already.

lnthach commented 1 year ago

@fkiraly first_diff (changed from firstdiff) behaves a bit differently though so I am not sure what is the best way to handle this parameter in sktime interface. In mrsqm, if first_diff is True then the features can be extracted from either the raw time series data or first difference (the chance is roughly 50%). This is, I believe, similarly to how some other classifiers (e.g. MultiRocket, Muse, Weasel 2) use the first difference. If first_diff is False then only raw data is used. A closer approximation with sktime would be something like sqm_with_first_diff = sqm + first_diff * sqm.

I am leaning toward including it in sktime so users can have more control. However it needs to be explained clearly to avoid confusion. I can add that in my notebook example. Do you think this works better ?

fkiraly commented 11 months ago

sure, that would work