Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG] statsforecast 1.6.0 compatibility - fix argument differences between sktime and statsforecast #5393

Merged
merged 2 commits into from Oct 9, 2023
Merged

[BUG] statsforecast 1.6.0 compatibility - fix argument differences between sktime and statsforecast #5393

merged 2 commits into from Oct 9, 2023

Conversation

luca-miniati
Copy link
Contributor

@luca-miniati luca-miniati commented Oct 9, 2023 •
edited

Reference Issues/PRs

Fixes #5295
Depends on #5317, merge after #5317

What does this implement/fix? Explain your changes.

Adds parameters stl_kwargs and pred_int_kwargs, keeping up with statsforecast new release

Did you add any tests for the change?

Added test params to StatsForecastMSTL, all tests in check_estimator pass

PR checklist

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned :-)
  • Optionally, I've added myself and possibly others to the CODEOWNERS file - 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.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 9, 2023

Thanks! Is this based on #5317, i.e., should #5317 be merged first?

@fkiraly fkiraly added module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting bugfix Fixes a known bug or removes unintended behavior labels Oct 9, 2023
@luca-miniati
Copy link
Contributor Author

Yes, it builds on #5317. Should I have made a PR to that fork instead?

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 9, 2023

Yes, it builds on #5317. Should I have made a PR to that fork instead?

Branching off the PR branch would have been cleaner, but as long as you say in the PR preamble "depends on #5317", it should be fine - so the other devs know which sequence this should be merged in.

@fkiraly fkiraly mentioned this pull request Oct 9, 2023
Merged
3 tasks
@fkiraly fkiraly changed the title [BUG] fix argument differences between sktime and statsforecast [BUG] statsforecast 1.6.0 compatibility - fix argument differences between sktime and statsforecast Oct 9, 2023
@fkiraly fkiraly merged commit c1fbe65 into sktime:main Oct 9, 2023
24 checks passed
@luca-miniati luca-miniati deleted the statsforecast-bugs branch October 9, 2023 23:09
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 10, 2023
@yarnabrina
Merge remote-tracking branch 'origin/main' into validate-extras-workflow
c31a6fe 
* origin/main:
  [MNT] [Dependabot](deps-dev): Update arch requirement from <6.2.0,>=5.6.0 to >=5.6.0,<6.3.0 (sktime#5392)
  [BUG] `statsforecast 1.6.0` compatibility - fix argument differences between `sktime` and `statsforecast` (sktime#5393)
  [BUG] `statsforecast 1.6.0` compatibility - in `statsforecast` adapter, fixing `RuntimeError: dictionary changed size during iteration` (sktime#5317)
  [DOC] fix typo in classification notebook (sktime#5390)
  [DOC] fix broken docstring example of `AlignerDtwNumba` (sktime#5374)
  [ENH] incremental testing to also test if any parent class in sktime has changed (sktime#5379)
  [ENH] simplified delegator interface to `dtw-python` based dynamic time warping distances (sktime#5348)
  [ENH] `YfromX` - probabilistic forecasts (sktime#5271)
  [ENH] removed py37.dockerfile and update doc entry for CI (sktime#5356)
  [ENH] lucky dynamic time warping distance and aligner (sktime#5341)
  [ENH] sensible default `_get_distance_matrix` for time series aligners (sktime#5347)
  [ENH] delegator for pairwise time series distances and kernels (sktime#5340)
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 10, 2023
@yarnabrina
Merge remote-tracking branch 'origin/main' into split-ci
2132e16 
* origin/main:
  [MNT] [Dependabot](deps-dev): Update arch requirement from <6.2.0,>=5.6.0 to >=5.6.0,<6.3.0 (sktime#5392)
  [BUG] `statsforecast 1.6.0` compatibility - fix argument differences between `sktime` and `statsforecast` (sktime#5393)
  [BUG] `statsforecast 1.6.0` compatibility - in `statsforecast` adapter, fixing `RuntimeError: dictionary changed size during iteration` (sktime#5317)
  [DOC] fix typo in classification notebook (sktime#5390)
  [DOC] fix broken docstring example of `AlignerDtwNumba` (sktime#5374)
  [ENH] incremental testing to also test if any parent class in sktime has changed (sktime#5379)
  [ENH] simplified delegator interface to `dtw-python` based dynamic time warping distances (sktime#5348)
  [ENH] `YfromX` - probabilistic forecasts (sktime#5271)
  [ENH] removed py37.dockerfile and update doc entry for CI (sktime#5356)
  [ENH] lucky dynamic time warping distance and aligner (sktime#5341)
  [ENH] sensible default `_get_distance_matrix` for time series aligners (sktime#5347)
  [ENH] delegator for pairwise time series distances and kernels (sktime#5340)
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 10, 2023
@yarnabrina
Merge remote-tracking branch 'origin/split-ci' into test-change-in-base
425ede2 
* origin/split-ci:
  control pytest configuration
  [MNT] [Dependabot](deps-dev): Update arch requirement from <6.2.0,>=5.6.0 to >=5.6.0,<6.3.0 (sktime#5392)
  [BUG] `statsforecast 1.6.0` compatibility - fix argument differences between `sktime` and `statsforecast` (sktime#5393)
  [BUG] `statsforecast 1.6.0` compatibility - in `statsforecast` adapter, fixing `RuntimeError: dictionary changed size during iteration` (sktime#5317)
  [DOC] fix typo in classification notebook (sktime#5390)
  [DOC] fix broken docstring example of `AlignerDtwNumba` (sktime#5374)
  [ENH] incremental testing to also test if any parent class in sktime has changed (sktime#5379)
  [ENH] simplified delegator interface to `dtw-python` based dynamic time warping distances (sktime#5348)
  [ENH] `YfromX` - probabilistic forecasts (sktime#5271)
  [ENH] removed py37.dockerfile and update doc entry for CI (sktime#5356)
  [ENH] lucky dynamic time warping distance and aligner (sktime#5341)
  [ENH] sensible default `_get_distance_matrix` for time series aligners (sktime#5347)
  [ENH] delegator for pairwise time series distances and kernels (sktime#5340)
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 10, 2023
@yarnabrina
Merge remote-tracking branch 'origin/split-ci' into test-change-in-fo…
8eb36b5 
…recasting

* origin/split-ci:
  control pytest configuration
  [MNT] [Dependabot](deps-dev): Update arch requirement from <6.2.0,>=5.6.0 to >=5.6.0,<6.3.0 (sktime#5392)
  [BUG] `statsforecast 1.6.0` compatibility - fix argument differences between `sktime` and `statsforecast` (sktime#5393)
  [BUG] `statsforecast 1.6.0` compatibility - in `statsforecast` adapter, fixing `RuntimeError: dictionary changed size during iteration` (sktime#5317)
  [DOC] fix typo in classification notebook (sktime#5390)
  [DOC] fix broken docstring example of `AlignerDtwNumba` (sktime#5374)
  [ENH] incremental testing to also test if any parent class in sktime has changed (sktime#5379)
  [ENH] simplified delegator interface to `dtw-python` based dynamic time warping distances (sktime#5348)
  [ENH] `YfromX` - probabilistic forecasts (sktime#5271)
  [ENH] removed py37.dockerfile and update doc entry for CI (sktime#5356)
  [ENH] lucky dynamic time warping distance and aligner (sktime#5341)
  [ENH] sensible default `_get_distance_matrix` for time series aligners (sktime#5347)
  [ENH] delegator for pairwise time series distances and kernels (sktime#5340)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fkiraly fkiraly fkiraly approved these changes

@achieveordie achieveordie Awaiting requested review from achieveordie achieveordie is a code owner

@benHeid benHeid Awaiting requested review from benHeid benHeid is a code owner

@yarnabrina yarnabrina Awaiting requested review from yarnabrina yarnabrina is a code owner

Assignees
No one assigned
Labels
bugfix Fixes a known bug or removes unintended behavior module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] latest releases of sktime and statsforecast are incompatible because of argument differences
2 participants
@luca-miniati @fkiraly