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.

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

[ENH] refactor _predict_moving_cutoff and bugfix, outer update_predict_single should be called #2466

Merged
merged 2 commits into from Apr 14, 2022
Merged

[ENH] refactor _predict_moving_cutoff and bugfix, outer update_predict_single should be called #2466

merged 2 commits into from Apr 14, 2022

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Apr 14, 2022 •
edited

This PR carries out a small refactor on _predict_moving_cutoff:

  • removal of code blocks conditional on return_pred_int=True, these were never reached
  • changing _update_predict_single call to update_predict_single, this was a bug, since this causes the cutoff to not be updated in _update_predict

@fkiraly fkiraly mentioned this pull request Apr 14, 2022
Merged
@fkiraly fkiraly added module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting bugfix Fixes a known bug or removes unintended behavior labels Apr 14, 2022
@fkiraly
Copy link
Collaborator Author

fkiraly commented Apr 14, 2022

@aiwalter, @GuzalBulatova, I think I finally found out what was causing all those test_update_predict_predicted_index failures with the repeated time index!

The default implementation of _predict_moving_cutoff was calling the private _update_predict instead of the public update_predict, which would cause to not update the cutoff, and hence resulting in repeated index and/or forecasts all made for the same time point.

Would appreciate your review on this.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Apr 14, 2022

PS: discovered by debugging this PR #2458, a refactor of a composite estimator. It seems, in hindsight, that this would always break test_update_predict_predicted_index of composites...

@aiwalter
Copy link
Contributor

oh nice! Then I can try to get rid of _update in the STLForecaster and see if your solution works

@fkiraly
Copy link
Collaborator Author

fkiraly commented Apr 14, 2022

oh nice! Then I can try to get rid of _update in the STLForecaster and see if your solution works

Your optimism is inspiring, as always.

@fkiraly fkiraly changed the title [ENH] refactor _predict_moving_cutoff [ENH] refactor _predict_moving_cutoff and bugfix, outer update_predict_single should be called Apr 14, 2022
@fkiraly fkiraly merged commit 6360a2b into main Apr 14, 2022
@fkiraly fkiraly deleted the update_predict_first_refactor branch April 14, 2022 09:40
@fkiraly fkiraly mentioned this pull request Apr 14, 2022
Merged
srggrs added a commit to Gridsight/sktime that referenced this pull request Apr 19, 2022
@srggrs
Merge remote-tracking branch 'upstream/main' into plateau-max-length
fce99a1 
* upstream/main:
  [ENH] more forecaster scenarios for testing: using `X` (sktime#2462)
  less uncertainty samples (sktime#2497)
  [ENH] remove error message on exogeneous X from DirRec reducer (sktime#2463)
  [BUG] fix accidental overwrite of default method/arg sequences in test scenarios (sktime#2457)
  changed references to fit-in-transform to fit_is_empty (sktime#2494)
  [MNT] loosen strict upper bound on `scipy` to 1.9.0 (sktime#2474)
  [DOC] Added clustering module to API docs (sktime#2429)
  [ENH] Add prediction intervals for `UnobservedComponets` forecaster (sktime#2454)
  [BUG] remove `alpha` arg from `_boxcox`, remove private method dependencies, ensure scipy 1.8.0 compatibility (sktime#2468)
  [BUG] removed metric integration notebook (sktime#2476)
  [ENH] refactor `_predict_moving_cutoff` and bugfix, outer `update_predict_single` should be called (sktime#2466)
  [ENH, BUG] Distance refactor and bug fix (sktime#2268)
  [ENH] add new argument return_tags to all_estimators (sktime#2410)
fkiraly added a commit that referenced this pull request Apr 29, 2022
@fkiraly
[ENH] removing dead args and functions post _predict_moving_cutoff …
f7a7c66 
…refactor (#2470)

Post #2466, there were dead args and functions, i.e., not called resp used by anything anymore. Both occurrences are in private locations, so can be removed without any adverse effect.

* `return_pred_int` args in `_predict_moving_cutoff` - would raise a `NotImplementedError`, which is moved upwards into the `_BaseWindowForecaster`
* `_format_moving_cutoff_prediction` in `_sktime.py` is an older version of the one in the base module, and not called anywhere.
@fkiraly fkiraly mentioned this pull request May 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aiwalter aiwalter aiwalter approved these changes

@mloning mloning Awaiting requested review from mloning

@TonyBagnall TonyBagnall Awaiting requested review from TonyBagnall

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
2 participants
@fkiraly @aiwalter