[ENH] Implements Out-of-Sample-Residual Wrapper for Sktime Forecasters #8936
Conversation
There was a problem hiding this comment.
FYI: @marrov @ankurankan @fkiraly
This is the draft implementation. Please take a look at _predict to see if it's going in the right direction. I am implementing other methods meanwhile. Also I've left a few quesition in the review please take a look at them as well.
| fh_in_sample_mask = fh._is_in_sample() | ||
| fh_out_of_sample_mask = fh._is_out_of_sample() |
There was a problem hiding this comment.
Is it safe to use these private methods of ForecastingHorizon, I didn't see them being used elsewhere.
There was a problem hiding this comment.
there are the public methods is_all_in_sample and is_all_out_of_sample, these are safer
| from sktime.split import CutoffSplitter | ||
|
|
||
| fh_in_sample = _y.shape[0] + fh_in_sample - 1 | ||
| cv = CutoffSplitter(fh_in_sample) |
There was a problem hiding this comment.
From discussion in the parent issue #8762 cv object is expected from the user, but how do we make sure that the user's cv is expected to created the right splits? CutoffSplitter cuts off the samples till the in-sample fh. Or I might be missing something here, please let me know.
There was a problem hiding this comment.
what do you mean by "the right splits"?
| preds_in_sample = self.forecaster_.update_predict( | ||
| _y, cv, X, update_params=False, reset_forecaster=True | ||
| ) |
There was a problem hiding this comment.
Do you think its fine to use update_predict here or should have it been a custom implementation? Because internally the fitted model, though it's using the correct context when predicting, yet it has seen the in-sample points during fit
There was a problem hiding this comment.
hm, then do we not need to pass less data to self_forecaster_ in its fit? Optimally, the points should not have already been seen in fit.
Reference Issues/PRs
Fixes #8762
Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
predict methods
Did you add any tests for the change?
Not yet
PR checklist
For all contributions
maintainerstag - do this if you want to become the owner or maintainer of an estimator you added.See here for further details on the algorithm maintainer role.
For new estimators
docs/source/api_reference/taskname.rst, follow the pattern.Examplessection.