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 ARIMARegressor #1894

Merged
merged 44 commits into from Mar 23, 2021
Merged

Add ARIMARegressor #1894

merged 44 commits into from Mar 23, 2021

Conversation

jeremyliweishih
Copy link
Contributor

Fixes #1498.

@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #1894 (ef7cc3c) into main (9b1ffde) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main    #1894     +/-   ##
=========================================
+ Coverage   100.0%   100.0%   +0.1%     
=========================================
  Files         274      276      +2     
  Lines       22360    22571    +211     
=========================================
+ Hits        22354    22565    +211     
  Misses          6        6             
Impacted Files Coverage Δ
evalml/pipelines/__init__.py 100.0% <ø> (ø)
evalml/pipelines/components/__init__.py 100.0% <ø> (ø)
evalml/pipelines/components/estimators/__init__.py 100.0% <ø> (ø)
evalml/tests/data_checks_tests/test_data_checks.py 100.0% <ø> (ø)
...alml/tests/model_family_tests/test_model_family.py 100.0% <ø> (ø)
evalml/utils/gen_utils.py 99.6% <ø> (ø)
evalml/model_family/model_family.py 100.0% <100.0%> (ø)
...valml/pipelines/components/estimators/estimator.py 100.0% <100.0%> (ø)
...lines/components/estimators/regressors/__init__.py 100.0% <100.0%> (ø)
...omponents/estimators/regressors/arima_regressor.py 100.0% <100.0%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b1ffde...ef7cc3c. Read the comment docs.

@ParthivNaresh ParthivNaresh self-assigned this Mar 13, 2021
@ParthivNaresh ParthivNaresh marked this pull request as ready for review March 18, 2021 15:39
Copy link
Collaborator

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

I agree with the comments left by @freddyaboulton and @bchen1116 , but think once those issues are addressed this is good to go.

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

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

Added some nit-picky comments, but the biggest thing before merge is, I don't think we should be adding this to _all_estimators_used_in_search?

docs/source/release_notes.rst Outdated Show resolved Hide resolved
evalml/tests/pipeline_tests/test_pipelines.py Outdated Show resolved Hide resolved
evalml/tests/component_tests/test_estimators.py Outdated Show resolved Hide resolved
evalml/tests/component_tests/test_arima_regressor.py Outdated Show resolved Hide resolved
evalml/tests/component_tests/test_components.py Outdated Show resolved Hide resolved
Comment on lines +97 to +98
p_error_msg = "ARIMA is not installed. Please install using `pip install statsmodels`."
arima = import_or_raise("statsmodels.tsa.arima.model", error_msg=p_error_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this in fit if we have it in __init__?

Copy link
Contributor

Choose a reason for hiding this comment

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

We end up using it a few lines below when we initialize the ARIMA model in different ways depending on if X is passed or not :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah what I meant was, is there a case where we catch this in fit when we didn't catch it in __init__? Can a user use fit without init? I'm assuming the use case is:

arima = ARIMARegressor(...) # will catch import error
arima.fit(...) # will we ever catch import error here?

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

@ParthivNaresh Thank you for making the changes! I think we need to make sure Arima is not used in automl search for the time being but other than that this is good to go!

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

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

Great work! I like the changes made!

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

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

Wanted to follow up about needing the import_or_raise statement in fit but otherwise, looks good to go! Thanks for making the changes, this looks really good 🥳

def __init__(self, date_column=None, trend='n', p=1, d=0, q=0,
random_seed=0, **kwargs):
"""
Arguments:
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 👏 👏

Comment on lines +97 to +98
p_error_msg = "ARIMA is not installed. Please install using `pip install statsmodels`."
arima = import_or_raise("statsmodels.tsa.arima.model", error_msg=p_error_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah what I meant was, is there a case where we catch this in fit when we didn't catch it in __init__? Can a user use fit without init? I'm assuming the use case is:

arima = ARIMARegressor(...) # will catch import error
arima.fit(...) # will we ever catch import error here?

@@ -159,7 +159,7 @@ def _get_subclasses(base_class):
'BaselineRegressionPipeline', 'ModeBaselineMulticlassPipeline', 'BaselineMulticlassPipeline',
'TimeSeriesBaselineRegressionPipeline', 'TimeSeriesBaselineBinaryPipeline',
'TimeSeriesBaselineMulticlassPipeline', 'KNeighborsClassifier',
'SVMClassifier', 'SVMRegressor'}
'SVMClassifier', 'SVMRegressor', 'ARIMARegressor'}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 👍 👍

@ParthivNaresh ParthivNaresh merged commit 6eaa1dc into main Mar 23, 2021
@freddyaboulton freddyaboulton deleted the js_1498_arima branch May 13, 2022 15:01
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.

Time series regression: Add ARIMA estimator
6 participants