-
Notifications
You must be signed in to change notification settings - Fork 86
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
Updated pipeline.get_prediction_intervals()
to add trend prediction interval information from STL decomposer
#4093
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4093 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 349 349
Lines 37588 37644 +56
=======================================
+ Hits 37469 37525 +56
Misses 119 119
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
d72d719
to
b8f784b
Compare
evalml/pipelines/components/transformers/preprocessing/stl_decomposer.py
Outdated
Show resolved
Hide resolved
be57e09
to
4be75cb
Compare
pipeline.get_prediction_intervals()
to add trend prediction interval information from STL decomposer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! Just a few comments about code conciseness and expected behavior.
evalml/pipelines/components/transformers/preprocessing/stl_decomposer.py
Outdated
Show resolved
Hide resolved
evalml/pipelines/components/transformers/preprocessing/stl_decomposer.py
Outdated
Show resolved
Hide resolved
… with unbuilt wheels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for refactoring some of that code out of _project_trend()
, definitely looks more concise. Left a few comments to address, but looking forward to getting this out!
if coverage is None: | ||
coverage = [0.95] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb question, and I'm guilty of doing this too, why not just have this be the default value in the function def?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
evalml/tests/component_tests/decomposer_tests/test_stl_decomposer.py
Outdated
Show resolved
Hide resolved
8719001
to
4acaa2b
Compare
has_stl = STLDecomposer.name in list( | ||
self.component_graph.component_instances.keys(), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, I suggested we use STLDecomposer.name
, but now on the heels of Jeremy's work I feel like we should follow his lead and check for any(isinstance(c, STLDecomposer) for c in self.component_graph.component_instances.values()
instead 😅 just to be safe
stl.fit(X_train, y_train) | ||
|
||
def assert_pred_interval_coverage(pred_interval): | ||
expected_coverage = [0.95] if coverage is None else coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check here that there aren't any other cover values that got added somehow? i.e. when expected_coverage = [0.95]
we only have the two columns?
Resolves #4060