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

Add get_prediction_intervals() at the pipeline level #4052

Merged
merged 14 commits into from
Mar 10, 2023

Conversation

christopherbunn
Copy link
Contributor

@christopherbunn christopherbunn commented Mar 6, 2023

Resolves #4060

Adds get_prediction_intervals() at the pipeline level, which runs inverse_transform() before returning results.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #4052 (8640605) into main (4c0ec4d) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #4052     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        349     349             
  Lines      37476   37514     +38     
=======================================
+ Hits       37358   37396     +38     
  Misses       118     118             
Impacted Files Coverage Δ
...omponents/estimators/regressors/arima_regressor.py 100.0% <ø> (ø)
...tors/regressors/exponential_smoothing_regressor.py 100.0% <ø> (ø)
...ponents/estimators/regressors/prophet_regressor.py 100.0% <ø> (ø)
...ponents/estimators/regressors/xgboost_regressor.py 100.0% <ø> (ø)
...valml/pipelines/components/estimators/estimator.py 100.0% <100.0%> (ø)
...s/components/estimators/regressors/et_regressor.py 100.0% <100.0%> (ø)
...s/components/estimators/regressors/rf_regressor.py 100.0% <100.0%> (ø)
...valml/pipelines/time_series_regression_pipeline.py 100.0% <100.0%> (ø)
.../tests/pipeline_tests/test_time_series_pipeline.py 100.0% <100.0%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor Author

@christopherbunn christopherbunn left a comment

Choose a reason for hiding this comment

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

Some considerations while reviewing:

@@ -172,6 +175,7 @@ def get_prediction_intervals(
nsimulations=X.shape[0],
repetitions=400,
anchor="end",
random_state=self.parameters["random_state"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up having to set the random_state here to the random_seed, otherwise we would not get deterministic results. I wonder if this needs to be updated for all of the sktime-based estimators?

Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to file!

Comment on lines +47 to +51
NO_PREDS_PI_ESTIMATORS = [
ModelFamily.ARIMA,
ModelFamily.EXPONENTIAL_SMOOTHING,
ModelFamily.PROPHET,
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the estimators where we currently can't use the recomposed predictions to calculate the prediction intervals.

Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

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

LGTM just a couple small fixes. We should also file an issue to update the user guide to use the pipeline level implementation since its more convinient!

@christopherbunn
Copy link
Contributor Author

@jeremyliweishih Updated the timeseries docs to use the pipeline implementation!

@christopherbunn christopherbunn enabled auto-merge (squash) March 8, 2023 21:36
Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

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

Nice work!

@christopherbunn christopherbunn merged commit 9d8082d into main Mar 10, 2023
@christopherbunn christopherbunn deleted the TML-6804-add-pl-get-ci branch March 10, 2023 18:02
@chukarsten chukarsten mentioned this pull request Mar 15, 2023
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.

pipeline.inverse_transform() is not ran when determining prediction intervals for decomposer pipelines
3 participants