Skip to content

DPSolveResult: record solution metadata; decide on Tv and mc cleanups #390

Description

@oyamad

Last remaining item of the QuantEcon.py feature-sync plan (#385/#386/#388/#389). QuantEcon.py's DPSolveResult records the solution method and the parameters used (method, epsilon, max_iter, k); the Julia DPSolveResult records none of these. While touching the struct, two pre-existing design wrinkles should be decided on (they were deferred from the #385 bugfix round): the Tv work buffer living in a results object, and the abstract, initially-undefined mc field.

Current state

DPSolveResult is defined in src/markov/ddp.jl (line ~461):

mutable struct DPSolveResult{Algo<:DDPAlgorithm,Tval<:Real}
    v::Vector{Tval}
    Tv::Vector{Tval}
    num_iter::Int
    sigma::Vector{Int}
    mc::MarkovChain
    # two inner constructors at lines ~468 and ~480, both calling
    # new{Algo,Tval}(v, similar(v), 0, similar(v, Int)) — note: mc is
    # left #undef and only assigned at the end of solve (lines ~756/~765)
end

Notes: the method is already recoverable from the Algo type parameter (DPSolveResult{PFI, Float64}), unlike in Python where it is a string field; mc::MarkovChain is an abstract type (MarkovChain is parametric), and accessing ddpr.mc before solve finishes throws UndefRefError; Tv is a solver work buffer, but it is public-facing — compute_greedy!(ddp, ddpr) writes into it and the test "compute_greedy! changes ddpr.v" reads it.

Implementation guide (minimal scope: metadata only)

  1. In src/markov/ddp.jl, add three fields to the struct after num_iter::Int: epsilon::Float64, max_iter::Int, k::Int. (See decision D1 below before choosing types.)
  2. Update both inner constructors (lines ~468 and ~480) to accept and pass the new values through new{Algo,Tval}(...). Keep the field order in the new call identical to the field order in the struct. Give the constructors keyword or positional arguments epsilon, max_iter, k — positional, placed after the existing arguments, is simplest.
  3. In both solve methods (lines ~751 and ~760), pass the effective values of epsilon, max_iter, k (i.e. after keyword defaults are applied) into the DPSolveResult constructor call.
  4. Update the DPSolveResult docstring's field list (one line per new field, ending with a period, per the docstring style guide in .github/copilot-instructions.md).
  5. Tests in test/test_ddp.jl (a new testset; use fresh local variable names inside the testset — see the scope pitfall in the instructions file): for each Algo in (VFI, PFI, MPFI), call res = solve(ddp0, Algo; max_iter=137, epsilon=1e-4, k=17) and assert res.max_iter == 137, res.epsilon == 1e-4, res.k == 17; also call with defaults and assert res.max_iter == 250, res.epsilon == 1e-3, res.k == 20.
  6. Run test/test_ddp.jl standalone, then the full test suite; check the benchmark suite still builds (julia --project=benchmark -e 'include("benchmark/benchmarks.jl")') since it constructs solve results.
  7. This is a struct-layout change like the one in ENH: Store s_indices; add num_sa_pairs, to_sa_pair_form, to_product_form #389: users of the public constructors are unaffected; mention it in the PR description anyway.

Decision calls needed

  • D1 — metadata field types. Python's result is dict-like, so each method stores only its relevant keys (PFI has no epsilon, VFI has no k). A Julia struct has fixed fields. Options: (a) store all three always, with the documented meaning "the value passed to (or defaulted by) solve, whether or not the method uses it" — simplest, type-stable, recommended; (b) Union{Float64,Nothing}-style fields mirroring Python's presence/absence — faithful but clunky. Also: epsilon::Float64 vs epsilon::Real (solve accepts epsilon::Real; storing as Float64 loses e.g. Rational epsilons — but tolerance exactness rarely matters; decide).
  • D2 — the Tv field. Options: (a) keep as is (non-breaking; a work buffer in a results object is odd but harmless); (b) remove it and let compute_greedy!(ddp, ddpr) allocate — breaking for code reading ddpr.Tv (including one existing test) and adds an allocation per call; (c) keep but document as internal. Recommendation: (a)/(c) now, revisit if the struct is ever redesigned.
  • D3 — the mc field. Options: (a) keep as is (abstract type + #undef until solve returns; UndefRefError for direct constructor users); (b) make DPSolveResult immutable and construct it fully at the end of solve — the clean design, but a bigger breaking change (every ddpr.v = ... mutation in _solve! must be reworked to local variables); (c) parameterize the mc field type — adds a type parameter, breaking for code naming DPSolveResult{Algo,Tval}. Recommendation: (a) for this issue; (b) is the right long-term shape if a breaking release is ever planned — record it, don't do it now.
  • D4 — method accessor. The method is encoded in the type parameter; for parity ergonomics, optionally add method(ddpr::DPSolveResult{Algo}) where Algo = Algo (or a Symbol). Decide whether this is worth exporting or is API noise; not required for the sync.
  • D5 — cross-language note. None of this changes solutions; the PR needs no benchmark run beyond the build check, but should carry the usual QuantEcon.py cross-reference (this is the Julia half of a two-sided convention; Python-side changes are not needed).

With D1(a), D2(a), D3(a), D4(skip), this is a small, mechanical PR — the guide above is complete for that path.

🤖 Generated with Claude Code (Claude Fable 5)

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions