Refactor ARIMA/Prophet #3104
Refactor ARIMA/Prophet #3104
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3104 +/- ##
=======================================
- Coverage 99.8% 99.7% -0.0%
=======================================
Files 315 315
Lines 30711 30547 -164
=======================================
- Hits 30620 30447 -173
- Misses 91 100 +9
Continue to review full report at Codecov.
|
| if y is None: | ||
| raise ValueError("ARIMA Regressor requires y as input.") | ||
| if X is not None: |
There was a problem hiding this comment.
This addresses #2960. The issue is that since TimeSeriesFeaturizer isn't included in ARIMA-based pipelines, the potential for target leakage can occur.
There was a problem hiding this comment.
I think we should back this out of this PR and tackle it in it's own issue because I don't think this is the best fix for the problem.
I think the root cause of the problem is including features that will not be known during prediction time during fit. And whether or not a feature is known-in-advanced is independent of it's correlation to the target (which is what TargetLeakeage is checking). It's possible that there is a feature that will not be known in advanced that is not picked up by TargetLeakage and it's also possible that a feature that will be known in advanced will be dropped because it's correlated with the target.
I think we gotta think this through a bit more it's possible #2960 is blocked by #2511
| X = X.select_dtypes(exclude=["datetime64"]) | ||
| fh_ = self._set_forecast(X) | ||
| X = X[self.cols_to_keep] | ||
| X = X.select_dtypes(exclude=["datetime64"]) |
There was a problem hiding this comment.
_remove_datetime isn't called here because ARIMA doesn't have a problem predicting on data that has an arbitrary temporal index, even if it was fit on data that doesn't have a temporal index (because the forecast horizon is relative to the end of the training data).
ParthivNaresh
changed the title
| if datetime_feature: | ||
| X_train["Dates"] = dates[:40] | ||
| X_test["Dates"] = dates[40:] | ||
| if train_none: |
There was a problem hiding this comment.
The difference between train_none and no_features is that train_none passes in None as X, while no_features passes in a dataframe with only an index set.
evalml/pipelines/components/estimators/regressors/arima_regressor.py
Outdated
Show resolved
Hide resolved
|
Just for completeness, I filed this : #3121 |
…nto ARIMA_DateIndex
ParthivNaresh
requested review from
angela97lin,
jeremyliweishih,
freddyaboulton,
bchen1116 and
eccabay
evalml/pipelines/components/estimators/regressors/arima_regressor.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@ParthivNaresh Thanks for this PR I think it looks great! I think it's awesome you added some new seasonal datasets from wikipedia for us to test on. Looking forward to the LG PR to add those so I can use them too. Can you upload the html report to your results page? I'm curious to hover over those datasets/look at the rankings table.
The one thing is that I think we should back out the changes related to #2960 and tackle that problem in its own PR.
evalml/pipelines/components/estimators/regressors/arima_regressor.py
Outdated
Show resolved
Hide resolved
| if y is None: | ||
| raise ValueError("ARIMA Regressor requires y as input.") | ||
| if X is not None: |
There was a problem hiding this comment.
I think we should back this out of this PR and tackle it in it's own issue because I don't think this is the best fix for the problem.
I think the root cause of the problem is including features that will not be known during prediction time during fit. And whether or not a feature is known-in-advanced is independent of it's correlation to the target (which is what TargetLeakeage is checking). It's possible that there is a feature that will not be known in advanced that is not picked up by TargetLeakage and it's also possible that a feature that will be known in advanced will be dropped because it's correlated with the target.
I think we gotta think this through a bit more it's possible #2960 is blocked by #2511
| ) | ||
| def test_fit_predict_ts_with_y_not_X_index( | ||
| mock_get_dates, mock_format_dates, ts_data_seasonal_train | ||
| def test_fit_predict_sk_failure( |
There was a problem hiding this comment.
nit: This is basically testing that without remove_datetime, our ARIMA component would fail right? Can we rename the test to reflect that and add a comment to the pytest.raises to explain why/what is happening?
There was a problem hiding this comment.
Actually this is testing a few different use cases in which the original sktime.AutoARIMA fails but our implementation succeeds.
- When the X index is of datetime type and the y index is not (and vice versa)
- When X has no features but still has an index
- When X has a datetime feature
So I named it test_fit_predict_sk_failure to represent the difference in behaviour from sktime and evalml. Also because of the different use cases, the error from sktime varies so I can't pin it down with an error message.
# Conflicts: # docs/source/release_notes.rst
…nto ARIMA_DateIndex
# Conflicts: # evalml/tests/component_tests/test_arima_regressor.py # evalml/tests/component_tests/test_prophet_regressor.py
Fixes: #3091 #3067 #3046
Perf tests here: https://alteryx.atlassian.net/wiki/spaces/PS/pages/1152516105/ARIMA+-+No+longer+relying+on+date+index
I've also added 4 new datasets to the S3 bucket:
I got these by downloading the Wikipedia page views from here for different articles over various time horizons.
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.rstto include this pull request by adding :pr:123.