[nicoloangileri]

ARMA.impulse_response and ARMA.simulation are shifted by p - q periods when the AR polynomial is longer than the MA polynomial. This includes every pure AR(p) with p >= 2 built with the default theta=0.

>>> import quantecon as qe
>>> qe.ARMA([0.5, -0.2]).impulse_response(5)
array([ 0.  ,  1.  ,  0.5 ,  0.05, -0.075])   # main
array([ 1.   ,  0.5  ,  0.05 , -0.075, -0.0475])   # this PR

The docstring says psi[0] is unity, and statsmodels' ArmaProcess(...).arma2ma gives the second line. qe.ARMA([0.5, 0.1, 0.05]) is delayed by two periods, and simulation starts with p - q exact zeros for the same reason.

The cause is in set_params: (ma_poly, ar_poly, 1) is passed to scipy.signal.dimpulse and dlsim, which read the coefficients as a transfer function in positive powers of z. When ar_poly is longer, the numerator has a lower degree and the system gets len(ar_poly) - len(ma_poly) pure delays. set_params already padded ar_poly when ma_poly was longer; it now also pads ma_poly with zeros, so both polynomials have the same length. spectral_density and autocovariance use freqz, which is not affected by trailing zeros, and their output is unchanged.

The existing tests use theta = np.zeros(3) with three AR coefficients, so the polynomials already had the same length there. The new tests check, for AR(1), AR(2), AR(3), ARMA(2, 1), ARMA(1, 2) and the existing test model:

• impulse_response against the MA(inf) coefficients from the recursion psi_j = theta_j + sum_i phi_i psi_{j-i};
• seeded simulation against sum_j psi_j u_{t-j} built from the same shocks.

Both fail on main and pass here. The full test suite passes locally (745 tests).

Related to #888 (test coverage of the simulation and impulse-response paths in _arma).

Assisted-by: Claude Code (Claude Opus 5.5)

[nicoloangileri]

Hi @mmcky, a gentle ping on this one when you have a moment. Happy to adjust anything if needed. Thanks!

[mmcky]

Thanks for this, and apologies for the slow reply. This is a correct and well-explained fix: I reproduced it locally, and both new tests fail on main and pass on this branch. We'll approve the CI run and get it in.

One quick request that we now make of every pull request. QuantEcon has adopted a Code of AI Use, which welcomes AI-assisted contributions and asks each pull request to say whether AI tools were used. Our pull-request template doesn't show this yet, so we're asking by hand. Could you:

1. Add one line to the description above: Assisted-by: <tool> (<model>) if AI tools meaningfully contributed (for example Assisted-by: Claude Code (Claude Fable 5.1)), or Assisted-by: none if they didn't.
2. Reply here to confirm that you chose the task, read the result, and will answer review on it.

We're asking everyone, so this isn't a flag on your pull request. Thanks!

[Copilot]

🔵 Needs a closer look

It intentionally changes the numeric output of two public library methods (impulse_response and simulation) for a released package, which warrants human sign-off even though the fix is correct and well-tested.

0 open findings

What changed in this PR

This PR fixes a latent bug in quantecon/_arma.py where ARMA.impulse_response and ARMA.simulation were shifted by p - q periods whenever the AR polynomial is longer than the MA polynomial (which includes every pure AR(p) with p ≥ 2 built with the default theta=0). The root cause is that set_params passes (ma_poly, ar_poly, 1) to scipy.signal.dimpulse/dlsim, which interpret the coefficients as a transfer function in powers of z; when the numerator (ma_poly) is shorter, the system acquires len(ar_poly) - len(ma_poly) pure delays. The previous code only padded ar_poly when ma_poly was longer; the fix symmetrically pads ma_poly as well so both polynomials match in length. spectral_density/autocovariance use freqz, which is unaffected by trailing zeros, so their output is unchanged.

Changes:

• Pad ma_poly with trailing zeros when ar_poly is longer, so impulse response/simulation are no longer delayed by p - q.
• Update the set_params docstring to document the equal-length requirement and rationale.
• Add two function-based tests validating impulse_response and seeded simulation against an independently-computed MA(∞) recursion across AR(1), AR(2), AR(3), ARMA(2,1), ARMA(1,2), and the existing test model.

File	Description
quantecon/​_arma.py	Symmetric zero-padding of ma_poly/ar_poly in set_params to eliminate the p − q delay; docstring updated to explain the equal-length requirement.
quantecon/​tests/​test_arma.py	New oracle helper _ma_coefficients plus two tests checking impulse-response and seeded simulation against the MA(∞) representation over several AR/ARMA orders.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[coveralls]

coverage: 94.055% (+0.06%) from 94.0% — nicoloangileri:fix/arma-impulse-response-lag into QuantEcon:main

[mmcky]

@oyamad once QEP-5 compliance lands this is looking 👍 to me.

[nicoloangileri]

Thanks for reproducing it! I've added the Assisted-by line: Claude Code helped find the root cause and write the fix and the tests. I chose the task, reviewed the diff and the tests myself, and will handle review on it.

[oyamad]

@nicoloangileri thanks, it now matches the Julia version:

julia> using QuantEcon

julia> println(impulse_response(ARMA([0.5, -0.2]); impulse_length=5))
[1.0, 0.5, 0.04999999999999999, -0.07500000000000001, -0.0475]

[nicoloangileri]

Hi @mmcky, @oyamad has approved this one and the QEP-5 line is in the description. Is there anything else needed before it can be merged? Thanks!

[mmcky]

thanks @nicoloangileri for your PR. Looks good to go 👍
(sorry been out of the office the past couple days).

Thanks @oyamad for review.

[mmcky]

@nicoloangileri thanks for the ping. Always appreciated.