Skip to content

Refactors TbatAdapter#1017

Merged
fkiraly merged 2 commits intosktime:mainfrom
thayeylolu:refactor-Tbatsadapter
Jun 23, 2021
Merged

Refactors TbatAdapter#1017
fkiraly merged 2 commits intosktime:mainfrom
thayeylolu:refactor-Tbatsadapter

Conversation

@thayeylolu
Copy link
Copy Markdown
Collaborator

Reference : #955

We need to slowly re-factor all forecasters to be compliant with the new interface (see PR #912). i.e., no overrides of fit etc and descendant classes only implementing core logic, while the BaseForecaster implements check etc logic.

Currently this is running due to various downwards compatibility workarounds, which should be removed once all forecasters are ready (in "no-override" state)

The forecasters that still need a downstream refactor are:

  • BATS, TBATS, and _Tbatsadapter

@thayeylolu thayeylolu marked this pull request as ready for review June 22, 2021 15:38
@thayeylolu thayeylolu requested a review from fkiraly as a code owner June 22, 2021 15:38
@thayeylolu
Copy link
Copy Markdown
Collaborator Author

@mloning Kindly review this too

@fkiraly fkiraly requested review from aiwalter and mloning June 23, 2021 10:17
Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

looks good to me.
Maybe we want to change the univariate-only tag (or not?), but I'd leave that either to @aiwalter or @mloning to directly commit, or later PR.

Copy link
Copy Markdown
Contributor

@aiwalter aiwalter left a comment

Choose a reason for hiding this comment

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

tbats package deals only with univariate, so its fine.

@fkiraly fkiraly merged commit 4d16dd4 into sktime:main Jun 23, 2021
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.

3 participants