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] fix STLForecaster tag ignores-exogenous-X to be correctly set for composites #5365

Merged
merged 4 commits into from Oct 7, 2023
Merged

[BUG] fix STLForecaster tag ignores-exogenous-X to be correctly set for composites #5365

merged 4 commits into from Oct 7, 2023

Conversation

yarnabrina
Copy link
Collaborator

@yarnabrina yarnabrina commented Oct 5, 2023 •
edited by fkiraly

Sets the ignores-exogenous-X tag of STLForecaster to False if and only if at least one of the used
forecaster has it as False, and True otherwise.

@yarnabrina yarnabrina added module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting bugfix Fixes a known bug or removes unintended behavior labels Oct 5, 2023
fkiraly
fkiraly requested changes Oct 5, 2023
View reviewed changes
Copy link
Collaborator

@fkiraly fkiraly left a comment •
edited

Choose a reason for hiding this comment

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

that didn't cause too much harm, did it?
It just passed X sometimes superfluously, so this is strictly speaking not a bug but a runtime improvement.

The class tag should still be False, as this stands for the "in general" case (in general, X is used).

fkiraly and others added 3 commits October 5, 2023 22:56
@yarnabrina
Copy link
Collaborator Author

that didn't cause too much harm, did it?

It was affecting me a bit. I was trying to inform users the choice of models and wanted to restrict based on tags of the models, e.g. if user expressed desire that they have exogenous features, I'll show them the ones that support them first with an option to see for other models that do not.

For now, I've kept my logic to set runtime tags for instances as before, but reverted the class tag value back to False to indicate in general it allows the ability to do so. I hope that's what you meant? Re-requesting review.

A question: why is this not part of "compose" forecasters?

fkiraly
fkiraly approved these changes Oct 6, 2023
View reviewed changes
Copy link
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.

but reverted the class tag value back to False to indicate in general it allows the ability to do so. I hope that's what you meant? Re-requesting review.

yes, exactly, thanks.

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 6, 2023

A question: why is this not part of "compose" forecasters?

Compose only has generic compositions in my mental model - this transformer makes specific model assumptions on the components.

Though I also do not think "trend" is the right place, I think probably it should be in one category together with LOESS or a vmd forecaster or similar.

@fkiraly fkiraly changed the title [BUG] STLForecaster always had ignores-exogenous-X tag as False irrespective of passed (or not passed) forecasters [BUG] fix STLForecaster tag ignores-exogenous-X to be correctly set for composites Oct 7, 2023
@fkiraly fkiraly merged commit a1ad02c into sktime:main Oct 7, 2023
24 checks passed
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 8, 2023
@yarnabrina
Merge remote-tracking branch 'origin/main' into validate-extras-workflow
14c26ec 
* origin/main:
  [MNT] skpro as a soft dependency (sktime#5273)
  [DOC] Correct code block formatting for pre-commit install command (sktime#5377)
  [ENH] parallelization backend calls in utility module - part 2, backend parameter passing (sktime#5311)
  [MNT] [Dependabot](deps): Bump stefanzweifel/git-auto-commit-action from 4 to 5 (sktime#5373)
  [MNT] python 3.12 compatibility - replace `distutils` calls with equivalent functionality (sktime#5376)
  [DOC] `sktime` intro notebook (sktime#3793)
  [ENH] Skip unnecessary fit in `ForecastX` if inner `forecaster_y` ignores `X` (sktime#5353)
  [ENH] refactor parallelization backend calls in utility module (sktime#5268)
  [ENH] ARCH model (arch package) Issue sktime#2173 (sktime#5326)
  [ENH] add error message return to `deep_equals` assert in `test_reconstruct_identical`  (sktime#4927)
  [DOC] Added all feature names to docstring for DateTimeFeatures class (sktime#5283)
  [BUG] fix `STLForecaster` tag `ignores-exogenous-X` to be correctly set for composites (sktime#5365)
  [BUG] Fix inconsistent date/time index in `plot_windows` sktime#4919 (sktime#5321)
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 8, 2023
@yarnabrina
Merge remote-tracking branch 'origin/main' into split-ci
1ec3790 
* origin/main: (24 commits)
  [MNT] skpro as a soft dependency (sktime#5273)
  [DOC] Correct code block formatting for pre-commit install command (sktime#5377)
  [ENH] parallelization backend calls in utility module - part 2, backend parameter passing (sktime#5311)
  [MNT] [Dependabot](deps): Bump stefanzweifel/git-auto-commit-action from 4 to 5 (sktime#5373)
  [MNT] python 3.12 compatibility - replace `distutils` calls with equivalent functionality (sktime#5376)
  [DOC] `sktime` intro notebook (sktime#3793)
  [ENH] Skip unnecessary fit in `ForecastX` if inner `forecaster_y` ignores `X` (sktime#5353)
  [ENH] refactor parallelization backend calls in utility module (sktime#5268)
  [ENH] ARCH model (arch package) Issue sktime#2173 (sktime#5326)
  [ENH] add error message return to `deep_equals` assert in `test_reconstruct_identical`  (sktime#4927)
  [DOC] Added all feature names to docstring for DateTimeFeatures class (sktime#5283)
  [BUG] fix `STLForecaster` tag `ignores-exogenous-X` to be correctly set for composites (sktime#5365)
  [BUG] Fix inconsistent date/time index in `plot_windows` sktime#4919 (sktime#5321)
  &benshamza, fkiraly [BUG] Fix `numba` errors when calling `tslearn` `lcss` (sktime#5368)
  [MNT] Dataset file restructure (sktime#5239)
  [BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed (sktime#5330)
  [MNT] [Dependabot](deps): Bump styfle/cancel-workflow-action from 0.11.0 to 0.12.0 (sktime#5355)
  [ENH] minor fixes to `NaiveAligner` (sktime#5344)
  [DOC] Improve Readability of Notebook 2 - Classification, Regression & Clustering (sktime#5312)
  [DOC] Documented ax argument and the figure in plot_series (sktime#5325)
  ...
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 8, 2023
@yarnabrina
Merge remote-tracking branch 'origin/split-ci' into test-change-in-fo…
2c35a9d 
…recasting

* origin/split-ci: (25 commits)
  added flags to codecov
  [MNT] skpro as a soft dependency (sktime#5273)
  [DOC] Correct code block formatting for pre-commit install command (sktime#5377)
  [ENH] parallelization backend calls in utility module - part 2, backend parameter passing (sktime#5311)
  [MNT] [Dependabot](deps): Bump stefanzweifel/git-auto-commit-action from 4 to 5 (sktime#5373)
  [MNT] python 3.12 compatibility - replace `distutils` calls with equivalent functionality (sktime#5376)
  [DOC] `sktime` intro notebook (sktime#3793)
  [ENH] Skip unnecessary fit in `ForecastX` if inner `forecaster_y` ignores `X` (sktime#5353)
  [ENH] refactor parallelization backend calls in utility module (sktime#5268)
  [ENH] ARCH model (arch package) Issue sktime#2173 (sktime#5326)
  [ENH] add error message return to `deep_equals` assert in `test_reconstruct_identical`  (sktime#4927)
  [DOC] Added all feature names to docstring for DateTimeFeatures class (sktime#5283)
  [BUG] fix `STLForecaster` tag `ignores-exogenous-X` to be correctly set for composites (sktime#5365)
  [BUG] Fix inconsistent date/time index in `plot_windows` sktime#4919 (sktime#5321)
  &benshamza, fkiraly [BUG] Fix `numba` errors when calling `tslearn` `lcss` (sktime#5368)
  [MNT] Dataset file restructure (sktime#5239)
  [BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed (sktime#5330)
  [MNT] [Dependabot](deps): Bump styfle/cancel-workflow-action from 0.11.0 to 0.12.0 (sktime#5355)
  [ENH] minor fixes to `NaiveAligner` (sktime#5344)
  [DOC] Improve Readability of Notebook 2 - Classification, Regression & Clustering (sktime#5312)
  ...
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Oct 8, 2023
@yarnabrina
Merge remote-tracking branch 'origin/split-ci' into test-change-in-base
0f337f7 
* origin/split-ci: (25 commits)
  added flags to codecov
  [MNT] skpro as a soft dependency (sktime#5273)
  [DOC] Correct code block formatting for pre-commit install command (sktime#5377)
  [ENH] parallelization backend calls in utility module - part 2, backend parameter passing (sktime#5311)
  [MNT] [Dependabot](deps): Bump stefanzweifel/git-auto-commit-action from 4 to 5 (sktime#5373)
  [MNT] python 3.12 compatibility - replace `distutils` calls with equivalent functionality (sktime#5376)
  [DOC] `sktime` intro notebook (sktime#3793)
  [ENH] Skip unnecessary fit in `ForecastX` if inner `forecaster_y` ignores `X` (sktime#5353)
  [ENH] refactor parallelization backend calls in utility module (sktime#5268)
  [ENH] ARCH model (arch package) Issue sktime#2173 (sktime#5326)
  [ENH] add error message return to `deep_equals` assert in `test_reconstruct_identical`  (sktime#4927)
  [DOC] Added all feature names to docstring for DateTimeFeatures class (sktime#5283)
  [BUG] fix `STLForecaster` tag `ignores-exogenous-X` to be correctly set for composites (sktime#5365)
  [BUG] Fix inconsistent date/time index in `plot_windows` sktime#4919 (sktime#5321)
  &benshamza, fkiraly [BUG] Fix `numba` errors when calling `tslearn` `lcss` (sktime#5368)
  [MNT] Dataset file restructure (sktime#5239)
  [BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed (sktime#5330)
  [MNT] [Dependabot](deps): Bump styfle/cancel-workflow-action from 0.11.0 to 0.12.0 (sktime#5355)
  [ENH] minor fixes to `NaiveAligner` (sktime#5344)
  [DOC] Improve Readability of Notebook 2 - Classification, Regression & Clustering (sktime#5312)
  ...
@yarnabrina yarnabrina deleted the fix-stl-tag branch October 9, 2023 04:01
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

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.

None yet
3 participants
@yarnabrina @fkiraly @benHeid