Skip to content

Forecast base class refactor and extension template#912

Merged
fkiraly merged 61 commits intomainfrom
forecast-base-refactor
Jun 12, 2021
Merged

Forecast base class refactor and extension template#912
fkiraly merged 61 commits intomainfrom
forecast-base-refactor

Conversation

@fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented May 30, 2021

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 _SktimeForecaster and the forecasting horizin mixins into BaseForecaster, 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_templates folder.

In terms of review, the key file is forecasting/base/_base, with corresponding changes (contraction and deletion) in base/_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, predict etc are overridden by the current descendants, as opposed to _fit and _predict.
The only change I had to made to descendants is set self._is_fitted=False at the start of fit, 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_fit tag (can be 'requried' or 'optional').
Temporarily, I've left both mixins and the _SktimeForecaster in and handled their behaviour as expected (inheriting from a mixin sets the fh_in_fit tag), 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 _SktimeForecaster in favour of a single BaseForecaster class.

@fkiraly fkiraly requested a review from mloning as a code owner May 30, 2021 00:12
@fkiraly fkiraly marked this pull request as draft May 30, 2021 00:13
@fkiraly fkiraly requested a review from big-o May 30, 2021 00:13
Copy link
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fkiraly this looks great, I had a quick look and left a few comments.

Copy link
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the suggested changes in #950, I'm happy to merge this as an intermediate step in the refactoring.

Copy link
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy for this to be merged now.

@fkiraly fkiraly merged commit ad82a27 into main Jun 12, 2021
@fkiraly fkiraly deleted the forecast-base-refactor branch June 12, 2021 18:40
@fkiraly
Copy link
Collaborator Author

fkiraly commented Jun 12, 2021

hm, the checks didn't finish after I merged your PR - I'd assume everything is still alright, @mloning?

@mloning
Copy link
Contributor

mloning commented Jun 12, 2021

@fkiraly in general please wait for the CI to complete before merging so that we don't end up breaking the main branch

@fkiraly
Copy link
Collaborator Author

fkiraly commented Jun 12, 2021

well, they did pass on your PR which was ahead of this one, so that technically counts? Will be more careful next time.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants