Repository navigation
fix(arima): keep seasonal period in search_arima fallback (#1276) - #1285
Ajay Mathuriya (ajaycodesitbetter) wants to merge 7 commits into
Conversation
|
Thanks for the fix! Could you also add an end-to-end test that goes through def test_auto_arima_nonstepwise_fallback_keeps_seasonal_period(monkeypatch):
original = arima_module.myarima
state = {"failed": False}
def myarima_refit_fails(*args, **kwargs):
fit = original(*args, **kwargs)
if kwargs.get("approximation") is False and not state["failed"]:
state["failed"] = True
fit = {**fit, "ic": math.inf}
return fit
monkeypatch.setattr(arima_module, "myarima", myarima_refit_fails)
fit = arima_module.auto_arima_f(
ap, period=12, stepwise=False, approximation=True, max_order=3
)
assert state["failed"]
assert fit["arma"][4] == 12 |
José Morales (jmoralez)
left a comment
There was a problem hiding this comment.
Thanks for the fix! I left a suggestion to simplify the tests a bit.
| def test_search_arima_fallback_keeps_seasonal_period(monkeypatch): | ||
| # when the non-approximate refit of the best model has infinite ic, | ||
| # search_arima falls back to a full search that must keep the seasonal period | ||
| rng = np.random.default_rng(0) | ||
| season = 12 | ||
| x = np.sin(np.arange(120) * 2 * np.pi / season) + rng.normal(size=120) * 0.3 | ||
| original = arima_module.myarima | ||
| state = {"failed": False} | ||
| seen_periods = [] | ||
|
|
||
| def myarima_refit_fails(*args, **kwargs): | ||
| seen_periods.append(kwargs["seasonal"]["period"]) | ||
| fit = original(*args, **kwargs) | ||
| if kwargs.get("approximation") is False and not state["failed"]: | ||
| state["failed"] = True | ||
| fit = {**fit, "ic": math.inf} | ||
| return fit | ||
|
|
||
| monkeypatch.setattr(arima_module, "myarima", myarima_refit_fails) | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore") | ||
| fit = arima_module.search_arima( | ||
| x, | ||
| max_p=1, | ||
| max_q=1, | ||
| max_P=1, | ||
| max_Q=1, | ||
| max_order=3, | ||
| approximation=True, | ||
| offset=0, | ||
| period=season, | ||
| ) | ||
| assert state["failed"] | ||
| # every model fitted, including those in the fallback search, uses the period | ||
| assert set(seen_periods) == {season} | ||
| assert fit["arma"][4] == season | ||
|
|
||
|
|
||
| def test_auto_arima_nonstepwise_fallback_keeps_seasonal_period(monkeypatch): | ||
| # end-to-end: the period given to auto_arima_f(stepwise=False) must survive | ||
| # the fallback search that search_arima runs when the exact refit fails | ||
| original = arima_module.myarima | ||
| state = {"failed": False} | ||
|
|
||
| def myarima_refit_fails(*args, **kwargs): | ||
| fit = original(*args, **kwargs) | ||
| if kwargs.get("approximation") is False and not state["failed"]: | ||
| state["failed"] = True | ||
| fit = {**fit, "ic": math.inf} | ||
| return fit | ||
|
|
||
| monkeypatch.setattr(arima_module, "myarima", myarima_refit_fails) | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore") | ||
| fit = auto_arima_f( | ||
| ap, period=12, stepwise=False, approximation=True, max_order=3 | ||
| ) | ||
| assert state["failed"] | ||
| assert fit["arma"][4] == 12 |
There was a problem hiding this comment.
The two tests repeat the same myarima wrapper, so this moves it into a fixture. It also limits the auto_arima_f grid to order 1 so the end-to-end test runs in ~1.5s instead of ~5s, checks that every model it fits uses period 12, and restores the blank lines before the next test block. Both tests still fail without the period=period fix.
| def test_search_arima_fallback_keeps_seasonal_period(monkeypatch): | |
| # when the non-approximate refit of the best model has infinite ic, | |
| # search_arima falls back to a full search that must keep the seasonal period | |
| rng = np.random.default_rng(0) | |
| season = 12 | |
| x = np.sin(np.arange(120) * 2 * np.pi / season) + rng.normal(size=120) * 0.3 | |
| original = arima_module.myarima | |
| state = {"failed": False} | |
| seen_periods = [] | |
| def myarima_refit_fails(*args, **kwargs): | |
| seen_periods.append(kwargs["seasonal"]["period"]) | |
| fit = original(*args, **kwargs) | |
| if kwargs.get("approximation") is False and not state["failed"]: | |
| state["failed"] = True | |
| fit = {**fit, "ic": math.inf} | |
| return fit | |
| monkeypatch.setattr(arima_module, "myarima", myarima_refit_fails) | |
| with warnings.catch_warnings(): | |
| warnings.simplefilter("ignore") | |
| fit = arima_module.search_arima( | |
| x, | |
| max_p=1, | |
| max_q=1, | |
| max_P=1, | |
| max_Q=1, | |
| max_order=3, | |
| approximation=True, | |
| offset=0, | |
| period=season, | |
| ) | |
| assert state["failed"] | |
| # every model fitted, including those in the fallback search, uses the period | |
| assert set(seen_periods) == {season} | |
| assert fit["arma"][4] == season | |
| def test_auto_arima_nonstepwise_fallback_keeps_seasonal_period(monkeypatch): | |
| # end-to-end: the period given to auto_arima_f(stepwise=False) must survive | |
| # the fallback search that search_arima runs when the exact refit fails | |
| original = arima_module.myarima | |
| state = {"failed": False} | |
| def myarima_refit_fails(*args, **kwargs): | |
| fit = original(*args, **kwargs) | |
| if kwargs.get("approximation") is False and not state["failed"]: | |
| state["failed"] = True | |
| fit = {**fit, "ic": math.inf} | |
| return fit | |
| monkeypatch.setattr(arima_module, "myarima", myarima_refit_fails) | |
| with warnings.catch_warnings(): | |
| warnings.simplefilter("ignore") | |
| fit = auto_arima_f( | |
| ap, period=12, stepwise=False, approximation=True, max_order=3 | |
| ) | |
| assert state["failed"] | |
| assert fit["arma"][4] == 12 | |
| @pytest.fixture | |
| def exact_refit_fails(monkeypatch): | |
| # makes the first non-approximate refit return an infinite ic, which forces | |
| # search_arima into its fallback search, and records every seasonal period fitted | |
| original = arima_module.myarima | |
| state = {"failed": False, "periods": set()} | |
| def myarima_refit_fails(*args, **kwargs): | |
| state["periods"].add(kwargs["seasonal"]["period"]) | |
| fit = original(*args, **kwargs) | |
| if kwargs.get("approximation") is False and not state["failed"]: | |
| state["failed"] = True | |
| fit = {**fit, "ic": math.inf} | |
| return fit | |
| monkeypatch.setattr(arima_module, "myarima", myarima_refit_fails) | |
| return state | |
| def test_search_arima_fallback_keeps_seasonal_period(exact_refit_fails): | |
| rng = np.random.default_rng(0) | |
| season = 12 | |
| x = np.sin(np.arange(120) * 2 * np.pi / season) + rng.normal(size=120) * 0.3 | |
| with warnings.catch_warnings(): | |
| warnings.simplefilter("ignore") | |
| fit = arima_module.search_arima( | |
| x, | |
| max_p=1, | |
| max_q=1, | |
| max_P=1, | |
| max_Q=1, | |
| max_order=3, | |
| approximation=True, | |
| offset=0, | |
| period=season, | |
| ) | |
| assert exact_refit_fails["failed"] | |
| assert exact_refit_fails["periods"] == {season} | |
| assert fit["arma"][4] == season | |
| def test_auto_arima_nonstepwise_fallback_keeps_seasonal_period(exact_refit_fails): | |
| with warnings.catch_warnings(): | |
| warnings.simplefilter("ignore") | |
| fit = auto_arima_f( | |
| ap, | |
| period=12, | |
| stepwise=False, | |
| approximation=True, | |
| max_p=1, | |
| max_q=1, | |
| max_P=1, | |
| max_Q=1, | |
| max_order=3, | |
| ) | |
| assert exact_refit_fails["failed"] | |
| assert exact_refit_fails["periods"] == {12} | |
| assert fit["arma"][4] == 12 | |
Fixes #1276.
When the non-approximate refit gives infinite ic, search_arima recursed without
period, so the fallback search used the default period=1 and returned a non-seasonal model. This passesperiod=periodin that recursive call.Adds a regression test that forces the fallback and checks that the seasonal period is kept for every model fitted (fails before the fix, passes after). The full tests/test_arima.py passes locally.
Note: the failing path is rare (stepwise=False, approximation=True, exact refit fails). I forced it with a mock in the test.
I used AI assistance to help write and verify this change; I reviewed it myself.