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 Exponential Smoothing time series estimator #3157

Merged
merged 30 commits into from
Jan 5, 2022

Conversation

eccabay
Copy link
Contributor

@eccabay eccabay commented Dec 16, 2021

Closes #2991. This will add the estimator to AutoML, as per the current perf test results.

@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #3157 (574be6e) into main (47d4867) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3157     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        324     326      +2     
  Lines      31302   31432    +130     
=======================================
+ Hits       31198   31328    +130     
  Misses       104     104             
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/component_tests/test_utils.py 95.7% <ø> (ø)
...alml/tests/model_family_tests/test_model_family.py 100.0% <ø> (ø)
evalml/model_family/model_family.py 100.0% <100.0%> (ø)
...lines/components/estimators/regressors/__init__.py 100.0% <100.0%> (ø)
...tors/regressors/exponential_smoothing_regressor.py 100.0% <100.0%> (ø)
...alml/tests/component_tests/test_arima_regressor.py 100.0% <100.0%> (ø)
evalml/tests/component_tests/test_components.py 99.3% <100.0%> (ø)
... and 4 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 47d4867...574be6e. Read the comment docs.

@eccabay eccabay self-assigned this Dec 16, 2021
random_seed=random_seed,
)

def _remove_datetime(self, data, features=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since _remove_datetime, _match_indices, and _set_forecast don't rely on the class instance and are shared by ARIMARegressor (and potentially future similar estimators), you can move these to pipelines/components/utils. Not necessary for this PR and we can file a separate issue for code cleanup if necessary.

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 here so far! There's some errors in the docstrings, and I believe a test should be fixed/updated for better coverage, but it's looking good!

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.

Left some comments, but exciting to be able to add this in soon!

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.

Thanks for making the changes! This looks great to me. Left one testing change suggestion, but not blocking!

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.

@eccabay Implementation looks good to me! Thanks for making the changes. Waiting on the latest batch of perf tests.

trend=None,
damped_trend=False,
seasonal=None,
sp=2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the default value of seasonal is None, shouldn't the default value of sp be None too? Is sp used only if seasonal is not None?

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 would have thought so as well, but there's something in the sktime implementation that enforces otherwise. I can't tell exactly where sp is used when seasonal is None, but it fails otherwise.

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.

A few nits and questions, but nothing blocking. Looks great! Good work!

return data_no_dt

def _set_forecast(self, X):
from sktime.forecasting.base import ForecastingHorizon
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we importing here to allow the import_or_raise to go first?

@@ -54,45 +54,6 @@ def test_fit_ts_without_y(ts_data):
clf.fit(X=X)


@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice, love this fixture reusage and combination.

assert not isinstance(y_train_removed.index, pd.DatetimeIndex)


def test_set_forecast(get_ts_X_y):
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems kind of weird to be testing what's going on purely internally to the regressor!

clf = ExponentialSmoothingRegressor()
with patch.object(clf, "_component_obj"):
clf.fit(X, y)
assert clf.feature_importance == np.zeros(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Filing this to standardize the way we handle these not-supported feature importance tests.

@eccabay eccabay merged commit 3ad0d3c into main Jan 5, 2022
@eccabay eccabay deleted the 2991_exponential_smoothing branch March 10, 2022 15:34
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.

Add exponential smoothing time series estimators
5 participants