PERF: Compute dense Q @ v with a single gemv in bellman_operator#857
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates DiscreteDP internals in quantecon.markov to (a) reduce overhead in dense Bellman updates by using a cached 2D view for a single GEMV, and (b) tighten correctness around several input-validation and iteration edge cases (notably for SA-pair indices and max_iter).
Changes:
- Optimize dense product-form
bellman_operatorby cachingQas a(n*m, n)view when C-contiguous (single GEMV) and cachingnp.arange(n)for state indexing. - Strengthen SA-pair construction validation (range checks, duplicate (s, a) detection, trailing-states-with-no-actions handling).
- Add/expand regression tests covering the new validations and the non-C-contiguous
Qfallback path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
quantecon/markov/ddp.py |
Adds cached dense Q 2D view for faster Bellman updates; adds/adjusts validation and iteration-edge-case behavior. |
quantecon/markov/utilities.py |
Fixes _generate_a_indptr to avoid out-of-bounds when trailing states have no SA pairs. |
quantecon/markov/tests/test_ddp.py |
Adds tests for trailing-state feasibility, duplicate SA pairs, out-of-range indices, max_iter validation, and non-C-contiguous Q fallback. |
Cache a 2-dimensional (n*m, n) view of Q at construction (when Q is C-contiguous) and use it in bellman_operator, so that the expected values are computed with one gemv call instead of a batched matmul over n stacked (m, n) matrices; measured 2.7x on the matvec and 1.5x on end-to-end value iteration at n=500, m=100. Non-C-contiguous Q falls back to the previous path (regression test added). Also cache the arange used for indexing by state in RQ_sigma and s_wise_max. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Caching the view at construction left bellman_operator computing with a stale array if a user rebound ddp.Q after construction (a pattern that worked before the gemv change), while RQ_sigma and evaluate_policy used the new one. Creating the reshape view per call costs nothing measurable next to the gemv and removes the staleness class entirely. Strengthen the non-contiguous-Q test with a nonuniform Q and a direct bellman_operator/greedy-policy comparison, and add a regression test for Q rebinding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mmcky
left a comment
There was a problem hiding this comment.
Validated locally on a clean worktree of the PR head — this is a clean, correct, no-behavior-change optimization. Approving. ✅
What I checked
- Full DDP test suite passes (
quantecon/markov/tests/test_ddp.py): 19 passed, including the two new regression tests (non-C-contiguousQfallback, andQrebinding). - Numerically bit-identical. The gemv over the 2-D
(n*m, n)view reproduces the batchedQ @ velement-for-element (allclose), andsolve('vi')values/policies match the fallback path exactly. SinceQis C-contiguous the reshape is a true view (rowi*m+j=Q[i,j,:]), so the equivalence is exact, not approximate. - Speedup reproduced. On
bellman_operatorat n=500, m=100 I measured ~2.2x here (batched matmul 4.5 ms → gemv 2.1 ms); consistent with the 2.7x reported (BLAS/machine-dependent). - Guards are sound.
if not self._sa_pair and self.Q.flags.c_contiguousshort-circuits before touching.flagsfor the sparse/sa-pair form, and a non-C-contiguousQ(where reshape would silently copy) correctly falls back to the original path. The contiguity check is a performance guard, not a correctness one. - Rebinding fix is real. Taking the 2-D view per call from
self.Qmeans a reboundddp.Qis picked up (covered by the new test); the earlier staleness class is gone. _s_arangecaching is consistent. Defined only in the non-sa-pair branch and referenced only from non-sa-pair paths (s_wise_max,RQ_sigma), wheren == num_states, so it's identical to the previous per-callnp.arange(num_states).
Nice work — the two dense representations are now performance-equivalent as intended, and the separate Krylov evaluate_policy idea is rightly left for its own issue.
One tiny housekeeping note (non-blocking): the PR description still mentions a cached self._Q_2d, but the final diff uses a per-call view — worth tidying that line so the description matches the merged code.
|
thanks @oyamad this looks great. Please merge when you're ready. |
|
@mmcky Thanks! (PR description edited (by Claude).) |
This PR is the Python side of the DDP performance pass (the Julia side is QuantEcon/QuantEcon.jl#386, merged). It contains no behavioral changes — all solutions are bit-identical.
Note: rebased on main after the merge of #855; the diff is now the PERF commit plus the review-fix commit (per-call view instead of a cached one).
Change
For the product formulation,
Q @ vwith the 3-dimensionalQis a batched matmul: n separate(m, n) @ (n,)products, with per-slice dispatch overhead. SinceQis C-contiguous in the standard case, the same computation is a single gemv over a 2-dimensional(n*m, n)view.bellman_operatornow takes that 2-D view per call whenQis C-contiguous (a view is free, and taking it per call means a reboundddp.Qis picked up — no cached state); a non-C-contiguousQ(where the reshape would silently copy) falls back to the previous path, with regression tests covering both paths andQrebinding. Thenp.arange(num_states)used for indexing by state inRQ_sigmaands_wise_maxis also cached instead of allocated per call.This also removes a historical asymmetry between the two dense representations: the state-action-pair form has always computed
Q @ vas a single gemv, which is why it used to outperform the product form by ~1.5-2x on identical data (visible e.g. in the 2015 benchmark notebook oyamad/mdp/ddp_performance.ipynb); with this change the two dense forms are performance-equivalent.Measured effect (identical random models across runs; values and iteration counts unchanged):
bellman_operator, n=500, m=100solve('vi'), n=500, m=100 (206 iter)bellman_operator, n=100, m=50solve('vi'), n=100, m=50PFI/MPFI improve slightly (14.1 → 11.8 ms and ~unchanged at n=500); the state-action-pair formulation is unaffected.
Considered and rejected, for the record
argmax+ fancy-index gather in the denses_wise_max: measured slower than NumPy (105 us vs 60 us at n=3000, m=50) — NumPy's SIMD-vectorizedargmaxbeats the branchy scalar loop. Kept the current implementation.evaluate_policy(permc_specvariants): at most 1.4x on a random-pattern test case — not worth the complexity.A finding worth separate discussion (not included here since it changes solver semantics): for the sparse formulation,
evaluate_policy's directspsolvesuffers badly from LU fill-in on scattered sparsity patterns, while Krylov methods exploit the structure ofA = I - beta Q_sigma— strictly diagonally dominant, condition number bounded by(1+beta)/(1-beta)— extremely well. On a random-pattern case with n=3000 and 5 nonzeros per row:spsolve522 ms vsbicgstab(rtol=1e-12)0.8 ms, accurate to ~1e-12. Since an iterative solve makes policy evaluation tolerance-based rather than direct (which touches policy iteration's exactness property), I'd propose it as an opt-in (e.g. a solver option) in a separate issue rather than a silent default change. Happy to open that issue if there is interest.🤖 Generated with Claude Code (Claude Fable 5)