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

Reduce use_covariates threshold #3868

Merged
merged 3 commits into from
Dec 1, 2022
Merged

Reduce use_covariates threshold #3868

merged 3 commits into from
Dec 1, 2022

Conversation

eccabay
Copy link
Contributor

@eccabay eccabay commented Nov 30, 2022

Moves to defaulting to use_covariates=False in more cases, since it produces significant slowdown without much benefit for larger datasets

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #3868 (332e3ba) into main (7438c15) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3868     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        344     344             
  Lines      36185   36191      +6     
=======================================
+ Hits       36048   36054      +6     
  Misses       137     137             
Impacted Files Coverage Δ
evalml/automl/automl_search.py 99.5% <ø> (ø)
...omponents/estimators/regressors/arima_regressor.py 100.0% <100.0%> (ø)
evalml/tests/automl_tests/test_automl.py 99.5% <100.0%> (+0.1%) ⬆️

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

@eccabay eccabay marked this pull request as ready for review November 30, 2022 20:33
Copy link
Collaborator

@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 suggestion

Copy link
Contributor

@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.

LGTM, just a question

@@ -61,6 +61,9 @@ class ARIMARegressor(Estimator):
supported_problem_types = [ProblemTypes.TIME_SERIES_REGRESSION]
"""[ProblemTypes.TIME_SERIES_REGRESSION]"""

max_rows = 1000
max_cols = 7
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker but is there a reason why this is 7 and not like 5 or 10? Just very curious haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, no strong motivation behind it. It was just a bit of a bump down, and without a more comprehensive set of datasets there was no way to test where the best line in the sand would be. Since these results seemed ok, I just stuck with it.

@eccabay eccabay merged commit e18dd14 into main Dec 1, 2022
@eccabay eccabay deleted the 450_use_covariates_threshold branch December 1, 2022 19:01
@christopherbunn christopherbunn mentioned this pull request Dec 8, 2022
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.

3 participants