Open fkiraly opened 3 years ago
@aiwalter, @mloning, would be interested in your opinion.
Sounds interesting, they could be combined into a "conditional refitter" with different options for what the condition is, i.e. in case 1 it's always true, in case 2 it's some duration threshold, in case 3 some error threshold.
The condition could perhaps even be expressable as an estimator itself (e.g. anomaly detector) - similar to the syntax and composition tools you find in adtk where we have operators for estimators (e.g. an aggregator). So you could express more complex rules like "refit if an anomaly was detected" given some anomaly detection estimator.
@mloning, yes, your idea sounds like an improvement over mine. Perhaps one "conditional refitter (simple)" and one "pipeline conditional refitter (adtk style)"?
"refit after time" is implemented as UpdateRefitsEvery
I'm interested in trying to do this if it's still relevant - I assume this would be implementing a couple new forecasters in the same sktime.forecasting.stream
package?
I'm interested in trying to do this if it's still relevant - I assume this would be implementing a couple new forecasters in the same
sktime.forecasting.stream
package?
Yes, or elsewhere if you think it makes more sense elsewhere.
"conditional refitter" idea of @mloning might be a good idea to start.
Thanks for the pointers! A few questions:
_update.py
don't seem to be covered by any tests, would it be worth it to add some?UpdateRefitsEvery
be refactored to use the new ConditionalRefitForecaster
(or ConditionalRefitter
)?ConditionalRefitForecaster
be public and usable by the users of just internal and used for subclassing? If it is the former we probably need some kind of API, maybe letting users pass in a few functions for 1) setting the state in _fit
, 2) deciding whether to refit in _update
, and 3) update the state before _update
is finished? We'll need something similar if it's just internal as well, of course, but having this as a public API might be a bit too complex? Added #3144 so we can discuss the API design with something concrete.The classes in
_update.py
don't seem to be covered by any tests, would it be worth it to add some?
Yes, tests are a good idea!
Note though, the forecasters in there are covered by tests, namely those in TestAllEstimators
and TestAllForecasters
. These are generic tests ensuring interface conformance with sklearn
and sktime
. But, they do not test functionality specific to a forecaster.
(to see which tests, run check_estimator
on the estimator)
So, tests for interface conformance (e.g., output has the right type) would be redundant, but checking whether the models really update at the right time, or other tests specific to the forecasters, would be useful, e.g,. for refactor.
Should
UpdateRefitsEvery
be refactored to use the newConditionalRefitForecaster
(orConditionalRefitter
)?
That may be a good idea. Although I think there is a trade-off to strike between simplicity and uniformity. I'm very open for you to make suggestions.
From a deprecation perspective, we need to keep UpdateRefitsEvery
as a class with its current functionality, at least for a deprecation cycle. Personally, I would not want to remove it, since it is simple to understand.
Should
ConditionalRefitForecaster
be public and usable by the users of just internal and used for subclassing? If it is the former we probably need some kind of API, maybe letting users pass in a few functions for 1) setting the state in_fit
, 2) deciding whether to refit in_update
, and 3) update the state before_update
is finished? We'll need something similar if it's just internal as well, of course, but having this as a public API might be a bit too complex? Added #3144 so we can discuss the API design with something concrete.
I think it would be useful with a public interface, although there may be a case for or against having a joint parent class.
If public, the condition would probably need to take the form of a function on the data? Or, some hard-coded conditions might also be useful (could be strings). Not entirely sure, keen to see your suggestions.
Is your feature request related to a problem? Please describe. In the Feb 2 meeting it was discussed that some behaviours in evaluation might actually be "business of the model" rather than of the evaluation. Irrespectively, even if evaluation workflows implement some of these behaviours, it might also be useful to have certain rules as behaviour modifying compositors for forecasters.
Describe the solution you'd like It might be useful to implement wrapper compositors that modify the behaviour of fitting/updating:
Forecaster
and replacesupdate
of the wrapped estimator with a sliding window re-fit. Hyper-parameters are sliding window parameters.Forecaster
and replacesupdate
with the following meta-algorithm: if time since lastfit
is <T, then callupdate
of wrapped forecaster. Otherwise, re-fit to a pre-specified window (fixed length or all past). Hyper-parameters are T and the window specs.Forecaster
and replacesupdate
with the following meta-algorihtm: re-fits if forecast error, evaluated in a certain way, exceeds a threshold; otherwise calls the wrappedupdate
. To check the condition, internally calls theevaluator
. Hyper-parameters are: evaluation parameters, including loss metric; threshold that needs to be passed for re-fit to be triggered. Could be dynamic, so might be interesting to think about how to make this more abstract to allow arbitrary smoothers/anomaly detectors on the loss metric.Describe alternatives you've considered The alternative would be to have "merged/contracted" estimators or composites in cases where these compositors might be applied instead, e.g., as hyper-parameter options of grid search tuners.