ENH: Make DPSolveResult immutable with solve metadata - #401
Open
oyamad wants to merge 2 commits into
Open
Conversation
Implements issue #390 with decisions D1(a), D2(c), D3(b), D4 skip, D5, plus the ddp back-reference pulled forward from the #398/#120 plan. DPSolveResult becomes an immutable struct DPSolveResult{Algo,Tval,TD<:DiscreteDP,TMC<:MarkovChain}, constructed complete at the end of solve, with new fields ddp (held by reference), epsilon, max_iter, and k (the options passed to or defaulted by solve). The solver internals go array-level: _solve! now has the signature _solve!(ddp, ::Type{Algo}, v, Tv, sigma, max_iter, epsilon, k) -> num_iter, and the five (ddp, ddpr)-taking methods (bellman_operator!, compute_greedy!, evaluate_policy, RQ_sigma, MarkovChain) are removed, replaced by their existing array-level twins plus the new MarkovChain(ddp, sigma). compute_greedy! is removed entirely (its only method was the (ddp, ddpr) one). New exported accessor controlled_mc(res) = res.mc returns the controlled Markov chain type-stably. Behavior-preserving for solution values, iteration counts, and convergence criteria. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the DiscreteDP solve-result container (DPSolveResult) in src/markov/ddp.jl to be immutable, fully constructed at the end of solve, and to carry uniform solve metadata (epsilon, max_iter, k) plus a back-reference to the solved model (ddp). It also removes the (ddp, ddpr)-style convenience methods in favor of the array/sigma-level APIs and adds a type-stable accessor for the controlled Markov chain.
Changes:
- Replaces the mutable, partially-initialized
DPSolveResultwith an immutable, fully-constructed result that storesddp,epsilon,max_iter, andk, and parameterizes the storedmctype for type-stable access. - Refactors
solveto allocate/seedv,Tv,sigmalocally, run_solve!on arrays, then constructDPSolveResultonce at the end; updates/modernizes internal_solve!signatures accordingly. - Removes the
(ddp, ddpr)method variants (e.g.bellman_operator!(ddp, ddpr),evaluate_policy(ddp, ddpr),MarkovChain(ddp, ddpr)), exportscontrolled_mc, and updates tests to cover metadata, immutability, and type stability.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/markov/ddp.jl |
Makes DPSolveResult immutable + fully constructed post-solve, adds solve metadata and ddp reference, refactors solve internals to array-based _solve!, and introduces MarkovChain(ddp, sigma) + controlled_mc. |
src/QuantEcon.jl |
Updates exports to drop compute_greedy! and export the new controlled_mc accessor. |
test/test_ddp.jl |
Replaces the old compute_greedy!-based test with an array-level bellman_operator! test and adds coverage for the new result metadata, immutability, and controlled_mc inference. |
Addresses review of the DPSolveResult rebuild. The hoisted PFI state-action buffer is now allocated at promote_type(eltype(R), eltype(v), typeof(beta)), matching what the 4-arg bellman_operator! the greedy step previously went through allocates each iteration. A buffer in eltype(v) rounds near-tied action values to equality before the argmax for mixed-precision models (e.g. Float32 rewards with a Float64 beta) and can flip the selected policy via the first-action tie-break. Regression test added; verified failing before this fix and matching master after it. VFI and MPFI keep their eltype(v) buffers, which is pre-existing behavior not changed by this PR. controlled_mc is removed before ever being released: it duplicated the documented and now concretely-typed res.mc field, and the name misreads as computing the chain from the current res.sigma, which is what MarkovChain(ddp, sigma) does. No replacement accessor is added here; a uniform accessor over results and the planned POMDPs.jl policy wrapper is deferred to the extension PR for #398. The `@inferred` pin on the concrete mc field remains via a test-local helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
oyamad
force-pushed
the
dpsolveresult-immutable
branch
from
July 26, 2026 08:05
8efcfb2 to
fe5aff5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #390, implementing decisions D1(a) (store
epsilon/max_iter/kuniformly), D2(c) (keepTv, documented as internal), D3(b) (immutable result constructed complete at the end ofsolve), D4 (nomethodaccessor), and D5 (see the cross-language note below), plus theddpback-reference from the #398/#120 plan so the parameter list lands in its final shape{Algo,Tval,TD,TMC}in a single breaking change.This is a breaking change; for the release notes:
DPSolveResultgains two type parameters (TD,TMC): exhaustiveDPSolveResult{Algo,Tval}annotations break;isa/dispatch on the partial form keeps working.DPSolveResultbecomes immutable:res.v = ...no longer compiles; mutating field contents (res.Tv .= ...) still does.ddp(the model, held by reference),epsilon,max_iter,k(the options passed to or defaulted bysolve).(ddp, ddpr)-taking methods are removed:bellman_operator!,compute_greedy!,evaluate_policy,RQ_sigma,MarkovChain. Migrations are one-liners to the array-level forms, e.g.bellman_operator!(ddp, res)→bellman_operator!(ddp, res.v, res.Tv, res.sigma);compute_greedy!is removed entirely (the(ddp, ddpr)method was its only one) and also leaves the export list. Note for downstream packages that extendcompute_greedy!as a generic (method extensions, not calls): the generic itself is deleted, so such packages should drop it from theirimport QuantEcon:lists and define their own function (this was the case for ContinuousDPs.jl, fixed there).DPSolveResult{Algo,Tval}(ddp[, v])are removed._solve!changes signature to_solve!(ddp, ::Type{Algo}, v, Tv, sigma, max_iter, epsilon, k) -> num_iter.New API:
MarkovChain(ddp, sigma), the controlled chain for a policy.Behavior-preserving otherwise: no solution values, iteration counts, or convergence criteria change.
Review revision (Codex/ChatGPT): the hoisted PFI state-action buffer initially used
eltype(v)and is now allocated atpromote_type(eltype(R), eltype(v), typeof(beta)), matching what the 4-argbellman_operator!previously allocated inside the greedy step each iteration; a narrowed buffer rounds near-tied action values to equality before the argmax for mixed-precision models (Float32 rewards with Float64 beta) and can flip the policy via the first-action tie-break. A regression test pins this (verified failing before the fix and agreeing withmasterafter). VFI/MPFI keep theireltype(v)buffers, which is pre-existingmasterbehavior (#386) deliberately not changed here. Also on review, the exported accessor initially included here (controlled_mc(res) = res.mc) was dropped: it duplicated the documented and now concretely-typedres.mcfield, and its name misreads as computing the chain from the currentres.sigma, which isMarkovChain(ddp, sigma)'s job. The@inferredpin on the concretemcfield remains via a test-local helper; a uniform accessor shared between results and the planned POMDPs.jl policy wrapper is deferred to the #398 extension PR.Lecture compatibility: the two lectures using
DiscreteDP(discrete_dp and aiyagari) were checked call by call against the removals above; no change to either lecture is needed. All their calls (DiscreteDPin both formulations,solvewith all three methods and keywords,results.v/.sigma/.num_iter/.mc,stationary_distributions(results.mc), the non-mutatingbellman_operator(ddp, w)) are on the surviving surface. The only visible difference is that thefieldnames(typeof(results))cell in discrete_dp now displays the nine fields of the new struct instead of five — the code runs unchanged.Performance (PkgBenchmark
judgevsmaster, plus the two lecture workloads): no measurable time change anywhere; PFI allocates onen×mstate-action buffer per iteration less (the buffer is now hoisted, as in VFI/MPFI), e.g. the Aiyagari lecture model (n=400, m=200, 17 PFI iterations) drops from 97.7 MiB to 87.7 MiB (−10.2%) per solve, andsa_sparse_n3000_m50_k5/solve_PFIin the benchmark suite from 221.0 MiB to 217.5 MiB. Downstream code additionally gains type stability onres.mcaccess, which was an abstract field before.Cross-reference to QuantEcon.py (D5): Python's
DPSolveResultdocumentsv, sigma, num_iter, mc, method, epsilon, max_iter; this PR adopts the same data-first/metadata-last convention. Python'sDiscreteDPmethods are array/sigma-taking throughout (bellman_operator(v, ...),evaluate_policy(sigma),controlled_mc(sigma)), so removing the Julia-only(ddp, ddpr)forms increases parity;MarkovChain(ddp, sigma)is the direct analogue ofcontrolled_mc(sigma). No Python-side change is needed.Verified: full test suite, the four validation scenarios in the repository instructions, the benchmark build check (
benchmark/ddp.jlalready uses the array forms exclusively), and a clean docs build.🤖 Generated with Claude Code (Claude Fable 5)