Forecast base class refactor and extension template#912
Merged
Conversation
mloning
approved these changes
Jun 12, 2021
Contributor
mloning
left a comment
There was a problem hiding this comment.
Happy for this to be merged now.
Collaborator
Author
|
hm, the checks didn't finish after I merged your PR - I'd assume everything is still alright, @mloning? |
Contributor
|
@fkiraly in general please wait for the CI to complete before merging so that we don't end up breaking the |
Collaborator
Author
|
well, they did pass on your PR which was ahead of this one, so that technically counts? Will be more careful next time. |
16 tasks
This was referenced Jun 15, 2021
Closed
fkiraly
added a commit
that referenced
this pull request
Jun 18, 2021
…veForecaster (#953) This is an exemplary refactor of a concrete estimator class, `NaiveForecaster`, to explore how general concrete forecaster refactors would work, along the lines discussed in #912. This PR changes: * `_BaseWindowForecaster` still inherits from `BaseForecaster` directly, and already looked extension spec compliant (no overrides, no tags) * `NaiveForecaster` inherits from `_BaseWindowForecaster`, and has been made extension spec compliant by adding the `requires-fh-in-fit` tag, and moving core logic from `fit` to `_fit`, while avoiding to override `fit` * it adds one line in `BaseForecaster.fit`, as a general change: the `cutoff` is set to the latest `y` index using `_set_cutoff`. This is done pre-`_fit`, which could override the set cut-off.
This was referenced Jun 21, 2021
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is addressing the forecaster base class proliferation discussed in #510, by simplifying the base class inheritance tree and the streamlining the private method logic.
Specifically, I've moved the logic from
_SktimeForecasterand the forecasting horizin mixins intoBaseForecaster, and adopted uniformly the principle of having a public method (fit,predict, etc) with checks/plumbing, dispatching to a "core logic" version (_fit,_predict, etc) where validated arguments can be assumed.The burden on extenders becomes much lighter, since it is now possible to only focus on the "core logic" implementation, instead of having to keep in mind a myriad of inconsistent and constantly shifting conventions in checks and other plumbing.
As side effects, if we get this right, this should make a few things on the roadmap easier:
As a proof-of-concept regarding ease of extension, this PR also contains a highly annotated extension template in the
extension_templatesfolder.In terms of review, the key file is
forecasting/base/_base, with corresponding changes (contraction and deletion) inbase/_sktime.I've tried to keep the interface consistent as much as possible (only changing internal logic).
Interface contracts with all the earlier estimators are still honoured, via loopthroughs and default behaviour that ensures that everything still works if
fit,predictetc are overridden by the current descendants, as opposed to_fitand_predict.The only change I had to made to descendants is set
self._is_fitted=Falseat the start offit, which is a minimally invasive change that's also separately reviewable as PR #941.There should only be a single change that breaks the tagging/inspection interface, which is replacing the choice of which forecasting horizon mixin one inherits from with a choice of the new
fh_in_fittag (can be'requried'or'optional').Temporarily, I've left both mixins and the
_SktimeForecasterin and handled their behaviour as expected (inheriting from a mixin sets thefh_in_fittag), so existing classes should still run, unless there is interface inconsistency or bugs in my refactor. Mid-term, we may want to remove the fh-mixins and_SktimeForecasterin favour of a singleBaseForecasterclass.