-
Notifications
You must be signed in to change notification settings - Fork 108
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] New base classes: base series estimator , segmenters and base series transformers #996
Conversation
# Conflicts: # aeon/segmentation/tests/test_all_segmenters.py
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.
The application of axis and its role to convert the time series format is clear to me, only minor documentation points identified, the rest LGTM !
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.
I can't really say I have considered everything, there is a lot going on in this PR.
One of my concerns is this will eventually all clash with forecasting design, where they usually do i.e. fit(y, X=None)
.
I dont intend making forecasters base series estimators series estimators any time soon, the interface is completely different, X and y dont mean the same things and they have a forecasting horizon parameter. |
but there is no reason not to have axis with forecasting, I would in any case have a BaseForecaster, where you could hard code axis to 1 |
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.
Didn't pick any problem, and considering the discussion about forecasting, I think we can move on with this PR for segmentation and transformation.
Maybe for a future PR, but one weird edge case that might be worth testing is the case where a different axis is given as input during fit and transform. From what I understand of the code, it shouldn't be a problem as you check for the axis defined in __init__
against the one given as parameter in _convert_X
, but better safe than sorry I guess ?
Great work !
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.
Yep think this is fine for now. Best to use it and build from there.
Part of #833
See #537
Fixes #1063
introduces a
BaseSeriesEstimator
along the lines ofBaseCollectionEstimator
.BaseSeriesEstimator
provides common checking and conversion tools. This was developed as successor to #946 andBaseSegmenter
will also extendBaseSeriesEstimator
when its finished.The motivation to do this is to side step all the excessive checking, conversion, so called "vectorisation" under the combined series and collection transformers, and to sort out the problem with different expectations as to the shape -
(m,d) vs (d,m)
series must be of type
np.ndarray, pd.Series, pd.DataFrame
. The major feature is that BaseSeriesEstimator has an axis. This means an estimator can be designed to treat a single series as either(n_timepoints, n_channels)
withaxis = 0
or (n_channels,n_timepoints) withaxis = 1
.The other point to note is that the tag
"capability:multivariate"
influences the conversion. If the tag is false, data passed in shape(n_timepoints,1)
or(1,n_timepoints)
is squeezed to be(n_timepoints,)
. If the tag is true, data passed as(n_timepoints,)
is expanded to be(n_timepoints,1)
or(1,n_timepoints)
dependent on axis.Matrix Profile converted to this format as first example. soon to be followed by a collection container. Need to put back the originals for deprecation