Add Support for Bool Features for ARIMA#3187
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3187 +/- ##
=======================================
+ Coverage 99.8% 99.8% +0.1%
=======================================
Files 326 326
Lines 31372 31392 +20
=======================================
+ Hits 31283 31303 +20
Misses 89 89
Continue to review full report at Codecov.
|
freddyaboulton
force-pushed
the
3181-arima-bool-features
branch
6 times, most recently
from
1a97244 to
4eff434
Compare
freddyaboulton
requested review from
ParthivNaresh,
angela97lin,
bchen1116,
christopherbunn,
chukarsten,
dsherry,
eccabay and
jeremyliweishih
and removed request for
ParthivNaresh and
eccabay
jeremyliweishih
left a comment
jeremyliweishih
left a comment
There was a problem hiding this comment.
Looks great to me! Just one question.
| X, y = self._manage_woodwork(X, y) | ||
| fh_ = self._set_forecast(X) | ||
| X = X.select_dtypes(exclude=["datetime64"]) | ||
| X = X.ww.select(exclude=["Datetime"]) |
There was a problem hiding this comment.
should we call self._remove_datetime here as well? Unsure if we need the other functionaility in self._remove_datetime
There was a problem hiding this comment.
Great question! I think we could but that would cause an extra copy so I'd rather keep as is?
There was a problem hiding this comment.
sounds good to me!
| ar._component_obj = MagicMock() | ||
| ar.fit(X, y) | ||
|
|
||
| pd.testing.assert_series_equal( |
freddyaboulton
force-pushed
the
3181-arima-bool-features
branch
from
4eff434 to
2c6475e
Compare
angela97lin
left a comment
angela97lin
left a comment
There was a problem hiding this comment.
LGTM, thank you @freddyaboulton ! Just left some comments on testing I was curious about :)
| X.ww.init() | ||
| X.ww["bool_1"] = ( | ||
| pd.Series([True, False]) | ||
| .sample(n=10, replace=True, random_state=0) |
There was a problem hiding this comment.
Extremely nit-picky but I wonder if we need any randomness here as opposed to just creating a numpy array of alternating true/false.
There was a problem hiding this comment.
We technically do not 😅
| ar._component_obj.fit.call_args[1]["X"]["bool_2"], X["bool_2"].astype(float) | ||
| ) | ||
|
|
||
| ar = ARIMARegressor(time_index="dates") |
There was a problem hiding this comment.
just wondering-- ar._component_obj = MagicMock() no longer applies to this line, right? Is that intentional? Are we just checking that we can actually fit/predict? I think this was not immediately clear to me. Could be clearer by just checking for these statements above when we mock, but I'm not sure 😅
If so, should we also check that predict doesn't return nan values?
There was a problem hiding this comment.
Yea the intention is to check that we can call non-mocked fit/predict on data with bools and not error out. I will add a comment and check for non-nan!
freddyaboulton
force-pushed
the
3181-arima-bool-features
branch
from
2c6475e to
4989517
Compare
freddyaboulton
force-pushed
the
3181-arima-bool-features
branch
from
4989517 to
280f231
Compare
4 participants
Pull Request Description
Fixes #3181
Perf tests:
report.html.zip
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.