-
Notifications
You must be signed in to change notification settings - Fork 84
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
For IterativeAlgorithm, put time series algorithms first #3407
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3407 +/- ##
=======================================
- Coverage 99.7% 99.7% -0.0%
=======================================
Files 332 332
Lines 32445 32510 +65
=======================================
+ Hits 32316 32380 +64
- Misses 129 130 +1
Continue to review full report at Codecov.
|
b72b8d0
to
d122a91
Compare
@@ -631,6 +633,22 @@ def __init__( | |||
self.data_splitter = self.data_splitter or default_data_splitter | |||
self.pipeline_parameters = pipeline_parameters or {} | |||
self.custom_hyperparameters = custom_hyperparameters or {} | |||
# Fitting takes a long time if the data is too wide or long. |
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.
if user_arima_hyperparams and not self.custom_hyperparameters[ | ||
ARIMARegressor.name | ||
].get("use_covariates"): | ||
self.custom_hyperparameters[ARIMARegressor.name].update( |
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.
Setting this up with custom hyperparameters is the best way I think. It lets users override our heuristic if they don't mind waiting.
@@ -5,7 +5,7 @@ | |||
|
|||
extras_require = { | |||
'update_checker': ['alteryx-open-src-update-checker >= 2.0.0'], | |||
'prophet': ['cmdstan-builder == 0.0.8'] | |||
'prophet': ['prophet-prebuilt == 1.0.2'] |
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.
Using pre-built wheels to speed up installation.
.tolist() | ||
) | ||
if is_using_windows: | ||
expected_order = [ |
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.
This is covered because windows test pass but we don't measure coverage for windows.
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.
Awesome Freddy, this looks great. You're going to make people very happy :)
@@ -39,7 +39,7 @@ jobs: | |||
name: Install evalml with test dependencies, core dependencies, & optional requirements (Prophet) | |||
run: | | |||
source test_python/bin/activate | |||
pip install cmdstan-builder==0.0.8 | |||
pip install prophet-prebuilt==1.0.2 |
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.
booo, I wanted prophet-freddy
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.
😂
"XGBoost Regressor", | ||
] | ||
else: | ||
expected_order = [ |
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.
Do we care about order? Probably a nit but I'd think checking that ESR, Prophet and ARIMA are simply in the list of the first few is what we care about. Feel free to ignore though. Just don't want to be changing this test over and over :)
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.
The good news is that adding a new model family won't break this test unless we explicitly change the _ESTIMATOR_FAMILY_ORDER
ef28fab
to
02d0728
Compare
Pull Request Description
Move time series estimators to the front of the queue for the iterative algorithm based on user feedback.
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.