Repository navigation
FIX: put the next-state expectation inside the inverse in solve_discrete_riccati_system - #983
Conversation
…ete_riccati_system
The Riccati update summed Π[i, j] * (...)' (Q + beta B' P_j B)^{-1} (...)
over next states j, so the expectation sat outside the inverse. The control
is chosen before the next state is known, so the update must use
P_bar = sum_j Π[i, j] P_j inside the inverse. This is the recursion that
LQMarkov.stationary_values already assumes when it forms F.
Add a regression test that checks Ps against the discounted cost of the
returned Fs and that perturbing Fs does not lower that cost. Regenerate the
TestLQMarkov reference values; they agree with an independent iteration of
the corrected recursion to the solver tolerance.
Fixes QuantEcon#982
Signed-off-by: Gaurav Jadhav <gaurav.vjadhav01@gmail.com>
|
Thanks for this, @jadhavgaurav, and for the clear write-up and regression test. One quick request that we now make of every pull request. You'll remember an earlier version of it from #964; the format has changed slightly since then. QuantEcon has adopted a Code of AI Use (QEP-5), which welcomes AI-assisted contributions and asks each pull request to say whether AI tools were used. Could you:
We're asking everyone, so this isn't a flag on your pull request. Thanks! |
|
Thanks @mmcky, I have added On the confirmation: I chose this issue (#982) from a list of candidates, the change and its tests were written by Claude Code under my direction, and I will deal with review feedback on this PR. The evidence for the change is the regression test and the independent NumPy and policy-iteration check described above, so please weigh it on those. Thanks for #984 too. If you want the extras from it on this PR (docstring, the |
There was a problem hiding this comment.
🟢 Approval recommended
The corrected recursion matches the issue's derivation and is consistent with the existing F formula, and I verified the regenerated scalar and matrix reference values exactly while a new regression test guards the fix.
0 open findings
What changed in this PR
This PR fixes a correctness bug (#982) in solve_discrete_riccati_system, the solver behind LQMarkov.stationary_values. The previous code averaged the quadratic correction term over next-period Markov states outside the matrix inverse, which is incorrect because the control is chosen before the next state is realized. The expectation must be taken inside the inverse via P_bar = Σ_j Π[i,j] P_j, reducing each state's update to the standard single-state Riccati update evaluated at P_bar. This aligns the value matrices Ps with the actual cost of the returned policy Fs, and makes Fs optimal. I numerically verified the regenerated reference values (both scalar and matrix cases) match the corrected recursion exactly.
Changes:
- Replaced the inner
sum1/sum2loop over next states with a singleP_bar-based Riccati update inside the inverse, and removed the now-unused buffers. - Regenerated reference values in four existing
TestLQMarkovtests to reflect the corrected recursion. - Added
test_stationary_P_is_cost_of_own_policy, a regression test verifyingPsequals the policy-evaluation fixed point ofFsand that perturbingFsnever lowers cost.
| File | Description |
|---|---|
quantecon/_matrix_eqn.py |
Moves the next-state expectation inside the inverse using P_bar; removes unused sum1/sum2 buffers. |
quantecon/tests/test_lqcontrol.py |
Updates reference P/d/F and sequence values for the corrected recursion; adds a policy-cost regression test for #982. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks @jadhavgaurav, both for adding the line and confirming, and for offering to port the extras. Yes please, that would be great! Your fix and #984's are the same at the core (both build the left factor directly, and the results agree up to rounding), and the regenerated reference values are identical, so only the additions need to come over. You can see them in #984's Files changed tab, or locally with:
A quick way to see Thanks again! |
Document the stacked Riccati equations in the solve_discrete_riccati_system docstring (raw string, return type ndim=3), and comment why the left factor of the correction term is formed directly. Add the _lq_markov_policy_cost helper with test_stationary_values_are_policy_costs and test_identical_regimes_match_lq. The latter fails when the left factor is formed as the transpose of the right factor. Signed-off-by: Gaurav Jadhav <gaurav.vjadhav01@gmail.com>
|
Ported the items from #984 in ae54500: the |
mmcky
left a comment
There was a problem hiding this comment.
Thanks @jadhavgaurav, and thanks for porting the extras from #984 so quickly. This looks good to me.
I checked the fix independently. The returned Ps satisfy the Markov-jump Bellman equation to about 1e-11 on both test fixtures and the #982 example. I checked this by minimising its right-hand side numerically rather than using the closed form. A separate policy-iteration solver also reproduces the regenerated reference values. On 785 random solvable problems, the result agrees with #984 to rounding, and with LQ on every single-regime one.
The one CI failure was an unrelated Nelder–Mead test with an unseeded starting point, and it passed on re-run.
Fixes #982.
solve_discrete_riccati_systemaveraged the quadratic correction over next states outside the matrix inverse. The control is chosen before the next state is known, so the expectation belongs inside: withP_bar = sum_j Π[i, j] P_j, the update is the single-state Riccati update evaluated atP_bar. This is the formLQMarkov.stationary_valuesalready uses forF, soFis now evaluated at the correctPs. The change is the replacement suggested in the issue; the now-unusedsum1/sum2buffers are removed.Tests
TestLQMarkov.test_stationary_P_is_cost_of_own_policy, built from the issue's reproducer. It checks thatPsequals the discounted cost of following the returnedFs(policy evaluation iterated to a fixed point), and that perturbingFsby +/-0.01 does not lower that cost in any state. It fails onmainand passes with the fix.test_stationary_scalar,test_stationary_mat,test_scalar_sequences_with_seedandtest_mat_sequencesare regenerated from the fixed code. I checked them against a separate NumPy iteration of the corrected recursion from the issue (max difference 2e-11 scalar case, 9e-10 matrix case, whosePentries reach 8e3). Thewandstatesequences are unchanged, as expected.LQMarkovandLQgive the sameP(difference about 1e-10).flake8 --select=F401,F405,E231 quanteconis clean andpytest quantecon/tests/test_lqcontrol.py quantecon/tests/test_matrix_eqn.pypasses (20 tests). I did not run the full suite.Assisted-by: Claude Code (Claude Sonnet 5.5)