re-factoring existing forecasters - Stacking, Multiplexer, Ensemble and TransformedTarget Forecasters by thayeylolu · Pull Request #965 · sktime/sktime

thayeylolu · 2021-06-15T14:16:34Z

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:

fkiraly-shell

[ removed, used the wrong account ]

fkiraly-shell

[ removed, used the wrong account ]

fkiraly

sorry, used the wrong account.

Looks good, but you need to add the estimator tags from the mixin to the concrete class, see the NaiveForecaster refactor.

thayeylolu · 2021-06-16T11:07:25Z

Thank you @fkiraly . the refracted NaiveForecaster uses _tags = {"requires-fh-in-fit": False}. But the extension_template uses _tags = { "fh_in_fit": False}. Which should I go with?

…udes tags

fkiraly · 2021-06-16T14:32:42Z

Which should I go with?

That depends, namely on the mixin the forecaster had inherited from before the refactor:

if it inherited from _OptionalForecastingHorizonMixin, it should get the tag "requires-fh-in-fit": False
if it inherited from _RequiredForecastingHorizonMixin, it should get the tag "requires-fh-in-fit": True

thayeylolu · 2021-06-16T15:49:12Z

Thank you @fkiraly

thayeylolu · 2021-06-17T09:58:57Z

Hi @fkiraly , I want to include the missing_data tag and the univariate tag in the forecaster's tag , but I honestly don't know if a forecaster can handle missing data or if it is univariate, unless it is stated in the doctstring. Is there a way I can go about this?

fkiraly · 2021-06-17T10:13:48Z

Hi @fkiraly , I want to include the missing_data tag and the univariate tag in the forecaster's tag , but I honestly don't know if a forecaster can handle missing data or if it is univariate, unless it is stated in the doctstring. Is there a way I can go about this?

That's a very good question.

I would recommend, always go with the "negative" version of the tag, i.e., assume the forecaster can do nothing. That way it will not break when the user tries to ask it to do something.

The "negative" version of the tags should be the ones in the extension guidelines.

If, despite this, the forecasters can deal with xyz, the I'd assume one of the authors or code owners will see this and comment that the "correct" tag should be different.

…d could not conform with the forecasting template

Lovkush-A · 2021-06-17T16:31:52Z

@thayeylolu @fkiraly

What is the plan for this PR? Is the plan for this single PR to update all the forecasters? If yes, I recommend changing the plan to split up the task into multiple PRs. The main reason is that it is easier to review and to debug small PRs than a single large PR.

fkiraly · 2021-06-17T17:21:58Z

I agree with @Lovkush-A - to make review and work easy, let's separate this into PR, @thayeylolu, along the lines of packages that are interfaced. E.g., all pmdarima in one PR, all tbats in one, etc.

The current changes can remain in here for the time being, but let's open new PR for other forecasters.

thayeylolu · 2021-06-17T18:08:08Z

Thank you @Lovkush-A . I also believe it is best to have multiple PRs for easier review.

fkiraly · 2021-06-18T14:07:18Z

@mloning, this is very odd - this PR didn´t do anything to time series classifiers, but that´s where the tests are now failing.

We initially suspected it had to do with the latest commits, so I suggested to create #977 which is without the latest changes.

Locally, according to @thayeylolu, the tests are not failing, which may indicate that this is perhaps due to the TSC code being affected by a package version change?

fkiraly · 2021-06-22T12:03:40Z

this looks done, do you want to update from main and make this a proper PR?

Lovkush-A · 2021-06-22T12:22:48Z

this looks done, do you want to update from main and make this a proper PR?

There are two version of this PR. #977 is the other one and I think 977 is the latest version.

thayeylolu · 2021-06-22T12:31:55Z

Yes. Thats why I closed this. I will have to write the reason for closing PRs in the future. Sorry for the misunderstanding

….4,>=3.3 to >=3.3,<3.5 (#5551) Updates the requirements on [pytest-xdist](https://github.com/pytest-dev/pytest-xdist) to permit the latest version. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pytest-dev/pytest-xdist/blob/master/CHANGELOG.rst">pytest-xdist's changelog</a>.</em></p> <blockquote> <h1>pytest-xdist 3.4.0 (2023-11-11)</h1> <h2>Features</h2> <ul> <li> <p><code>[#963](pytest-dev/pytest-xdist#963) <https://github.com/pytest-dev/pytest-xdist/issues/963></code>_: Wait for workers to finish reporting when test run stops early.</p> <p>This makes sure that the results of in-progress tests are displayed. Previously these reports were being discarded, losing information about the test run.</p> </li> <li> <p><code>[#965](pytest-dev/pytest-xdist#965) <https://github.com/pytest-dev/pytest-xdist/issues/965></code>_: Added support for Python 3.12.</p> </li> </ul> <h1>pytest-xdist 3.3.1 (2023-05-19)</h1> <h2>Bug Fixes</h2> <ul> <li> <p><code>[#907](pytest-dev/pytest-xdist#907) <https://github.com/pytest-dev/pytest-xdist/issues/907></code>_: Avoid remote calls during startup as <code>execnet</code> by default does not ensure remote affinity with the main thread and might accidentally schedule the pytest worker into a non-main thread, which breaks numerous frameworks, for example <code>asyncio</code>, <code>anyio</code>, <code>PyQt/PySide</code>, etc.</p> <p>A more safe correction will require thread affinity in <code>execnet</code> (<code>pytest-dev/execnet#96 <https://github.com/pytest-dev/execnet/issues/96></code>__).</p> </li> </ul> <h1>pytest-xdist 3.3.0 (2023-05-12)</h1> <h2>Features</h2> <ul> <li><code>[#555](pytest-dev/pytest-xdist#555) <https://github.com/pytest-dev/pytest-xdist/issues/555></code>_: Improved progress output when collecting nodes to be less verbose.</li> </ul> <h1>pytest-xdist 3.2.1 (2023-03-12)</h1> <h2>Bug Fixes</h2> <ul> <li><code>[#884](pytest-dev/pytest-xdist#884) <https://github.com/pytest-dev/pytest-xdist/issues/884></code>_: Fixed hang in <code>worksteal</code> scheduler.</li> </ul> <h1>pytest-xdist 3.2.0 (2023-02-07)</h1> <p>Improved Documentation</p>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/a6b56112f9b686aed7a354e0d50ecc26ad2d9dfe"><code>a6b5611</code></a> Update CHANGELOG</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/64f9388f4164e9f64d98a7580933e958ea4a09d5"><code>64f9388</code></a> Add support for Python 3.12</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/230ba6ad1057574c9f3d42a97f890788cd9ec6c3"><code>230ba6a</code></a> Properly wait for workers when test run terminates early (<a href="https://redirect.github.com/pytest-dev/pytest-xdist/issues/963">#963</a>)</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/93ca202b400eecc62919bcd9b0bab001e3f3f7ef"><code>93ca202</code></a> fix typo index -> instead</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/8f3d1ad92f02a251bcae5dc648a70c3f30ece364"><code>8f3d1ad</code></a> [pre-commit.ci] pre-commit autoupdate</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/ab3d6a9ee5aa4145fde339823ab1605c02f28ba4"><code>ab3d6a9</code></a> Merge pull request <a href="https://redirect.github.com/pytest-dev/pytest-xdist/issues/955">#955</a> from pytest-dev/pre-commit-ci-update-config</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/a5210d41034b2f48f2df05d2b62b0fb3495f346f"><code>a5210d4</code></a> [pre-commit.ci] pre-commit autoupdate</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/0b6b9c3c3858d05c353a14b29c58c8c3f63ae437"><code>0b6b9c3</code></a> Merge pull request <a href="https://redirect.github.com/pytest-dev/pytest-xdist/issues/954">#954</a> from pytest-dev/pre-commit-ci-update-config</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/838434c269a225bef167fa4314df905b97b9e958"><code>838434c</code></a> Install requirements.txt while building docs</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/b85a4717f96e2b40910aaa289428671779cb6ecb"><code>b85a471</code></a> Add docs/requirements.txt</li> <li>Additional commits viewable in <a href="https://github.com/pytest-dev/pytest-xdist/compare/v3.3.0...v3.4.0">compare view</a></li> </ul> </details> <br /> Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

refactors ensemble and transformedtargetforecaster

1856817

fkiraly-shell approved these changes Jun 16, 2021

View reviewed changes

fkiraly-shell suggested changes Jun 16, 2021

View reviewed changes

sktime deleted a comment from fkiraly-shell Jun 16, 2021

fkiraly self-requested a review June 16, 2021 10:12

fkiraly requested changes Jun 16, 2021

View reviewed changes

refactors ensemble, multiplexer and transformedtargetforecaster. Incl…

2ed978c

…udes tags

refactor StackingForecaster

9db81b2

include doctstring in stack and made update method private

f21f118

fkiraly mentioned this pull request Jun 17, 2021

Exemplary concrete estimator refactor post interface refactor, of NaiveForecaster #953

Merged

docstring changes to all except _reduce.py which has minor changes an…

d364383

…d could not conform with the forecasting template

thayeylolu changed the title ~~re-factoring existing forecasters to new interface~~ re-factoring existing forecasters - Stacking, Multiplexer, Ensemble and TransformedTarget Forecasters Jun 18, 2021

thayeylolu added 2 commits June 18, 2021 11:28

removes changes in _reduce.py

6acd521

removes changes in _reduce.py

31f372c

fkiraly mentioned this pull request Jun 19, 2021

Forecasting support for multivariate y and multiple input/output types - working prototype #980

Merged

Merge branch 'main' into refactor-forecasters

9104b73

fkiraly requested review from aiwalter and mloning June 22, 2021 12:02

thayeylolu closed this Jun 22, 2021

Uh oh!

Comments

Conversation

thayeylolu commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fkiraly-shell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fkiraly-shell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fkiraly left a comment

Choose a reason for hiding this comment

Uh oh!

thayeylolu commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fkiraly commented Jun 16, 2021

Uh oh!

thayeylolu commented Jun 16, 2021

Uh oh!

thayeylolu commented Jun 17, 2021

Uh oh!

fkiraly commented Jun 17, 2021

Uh oh!

Lovkush-A commented Jun 17, 2021

Uh oh!

fkiraly commented Jun 17, 2021

Uh oh!

thayeylolu commented Jun 17, 2021

Uh oh!

fkiraly commented Jun 18, 2021

Uh oh!

fkiraly commented Jun 22, 2021

Uh oh!

Lovkush-A commented Jun 22, 2021

Uh oh!

thayeylolu commented Jun 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thayeylolu commented Jun 15, 2021 •

edited

Loading

fkiraly-shell left a comment •

edited

Loading

fkiraly-shell left a comment •

edited

Loading

thayeylolu commented Jun 16, 2021 •

edited

Loading