ENH: Support sparse output in random_stochastic_matrix, random_markov_chain - #394
Merged
Conversation
…_chain
Add a keyword argument sparse::Union{Val{true},Val{false}}=Val(false)
to random_markov_chain and random_stochastic_matrix. With Val(true),
the matrix is built directly as a SparseMatrixCSC from the sampled
(row, column, value) triplets instead of being materialized densely.
The Val-typed flag keeps the methods type stable (tested with
@inferred), which a plain Bool keyword could not.
The random number stream is unchanged, and for a given rng the dense
and sparse formats sample the identical matrix: the nonzero-position
sampling is shared in _random_nonzero_indices, which consumes no
random numbers if k == n.
Closes #102.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The private random_stochastic_matrix in benchmark/mc_tools.jl shadowed the QuantEcon export of the same name; prefix both generators with mc_ (as done for new_mc_rng) and document why they remain local instead of using the package generators. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the Markov-chain random model utilities to optionally produce sparse transition matrices, enabling users to generate SparseMatrixCSC output without first materializing dense matrices, while preserving the RNG stream between dense and sparse formats. It also renames benchmark-local matrix generators to avoid shadowing the exported random_stochastic_matrix.
Changes:
- Add a
sparse::Union{Val{true},Val{false}}=Val(false)keyword torandom_stochastic_matrixandrandom_markov_chain, constructing sparse output directly from sampled triplets. - Add tests validating type stability, nonzero counts, and identical sampled matrices between dense and sparse outputs for fixed seeds.
- Rename benchmark-local stochastic matrix generators to
mc_-prefixed names to avoid shadowing public exports.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/markov/random_mc.jl |
Adds sparse=Val(true) support and factors nonzero-index sampling into a shared helper. |
test/test_random_mc.jl |
Adds coverage for sparse-output behavior and RNG-stream equivalence with dense output. |
benchmark/mc_tools.jl |
Renames benchmark-local generators to avoid shadowing exported API names. |
- Remove the brackets from `[rng]` in the `_random_nonzero_indices` docstring: there is no method without an explicit rng - Fix the keyword default notation for `sparse` to `=Val(false)`, matching the style of the surrounding entries - Test that the dense and sparse formats leave the rng stream in the same state, not only that they sample the same matrix - Cover sparse=Val(true) in the k=1 testset Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
oyamad
added a commit
that referenced
this pull request
Jul 12, 2026
After the rebase on #394, the unprefixed name resolves to the package export of random_stochastic_matrix rather than the local generator, against the stated independence of the benchmark models from markov/random_mc.jl. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
oyamad
added a commit
that referenced
this pull request
Jul 12, 2026
* PERF: Cache transition CDFs on MarkovChain; add sparse simulate path
- Cache the transition CDFs on the MarkovChain instance, built lazily
on the first simulation call. The previous code rebuilt them on
every simulate call, via a lazy adjoint whose columns are stride-n
views (a pre-0.7 "transpose then view" optimization that Matrix(p)'
silently voided). The cache field is typed Union{Nothing,TS} with TS
a new type parameter computed from the matrix type, so the sampler
access and the simulation entry points are type stable (tested with
@inferred). Assigning a new matrix to mc.p invalidates the cache
(Base.setproperty!); in-place modification of the matrix does not,
which is documented.
- Add a dedicated sampler for SparseMatrixCSC transition matrices,
sampling from per-row nonzero-entry CDFs (stored as the materialized
transpose) instead of densifying the matrix.
- The samplers fall back to the last state with positive transition
probability when a draw exceeds the final CDF value by rounding
(the previous DiscreteRV-based path could emit the out-of-range
index n+1 with probability of the order of the row-sum deficiency).
- recurrent_classes: return the single recurrent class directly for
strictly positive dense matrices, which are irreducible, instead of
building the graph (~1ms of the 1.8ms of stationary_distributions
at n=200).
- Remove dead pre-0.7 commented iteration code.
Benchmarks (SUITE["mc_tools"], minimum times, vs #391 baseline):
simulate/dense_n1000_ts100 928us -> 3.6us; sparse_n1000_k4_ts10000
1.83ms -> 138us; stationary_distributions/dense_n200 1.81ms -> 844us;
dense_n100_ts10000 307us -> 263us. All now faster than warm-cache
QuantEcon.py (10.4us / 191us / 925us / 296us).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Add cold-cache simulate and reducible-chain benchmark cases
The shared-instance simulate cases now measure the warm-cache steady
state; add _cold variants (fresh chain per sample, timing the CDF-cache
construction) and a reducible stationary_distributions case that
exercises the graph path bypassed by strictly positive matrices, as
suggested in the #391 review. Document the warm/cold semantics in the
README.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Remove the MarkovChain CDF cache; build samplers per call
Simulation calls on a shared MarkovChain must stay safe to run
concurrently — the natural threaded Monte Carlo pattern — which the
lazily initialized cache field broke: the first simulate call on each
thread raced on setfield!. Thread-safe lazy initialization needs
atomic fields and deserves its own design discussion; deferred to
#396.
The per-call construction cost is negligible for the sparse sampler
and on par with the previous DiscreteRV-based path for the dense one,
so the other improvements are kept: the dedicated sparse sampler, the
CDF-search overflow guard, and the contiguous dense CDFs.
With the cache go the sampler type parameter of MarkovChain, the
setproperty! override, and the warm/cold benchmark split.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* FIX: Return Int states from the sparse sampler
draw_next returned the raw rowvals entry, whose type is the index
type Ti of the SparseMatrixCSC. For Ti != Int, the iterator state of
MCIndSimulator, annotated Tuple{Int,Int}, rejected the second step
with a MethodError. Convert at the boundary; a state index always
fits in Int.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Use the local generator in the reducible benchmark case
After the rebase on #394, the unprefixed name resolves to the package
export of random_stochastic_matrix rather than the local generator,
against the stated independence of the benchmark models from
markov/random_mc.jl.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Drop the Real constraint on the sampler eltypes
MarkovChain does not constrain the eltype of its transition matrix,
and neither did the DiscreteRV path, so an abstractly typed matrix
(e.g. Matrix{Number} with real entries) constructed but no longer
simulated. The samplers work with any eltype that cumsum and
searchsortedfirst accept; Ti<:Integer stays, matching the constraint
of SparseMatrixCSC itself.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
This PR adds a keyword argument
sparse::Union{Val{true},Val{false}}=Val(false)torandom_markov_chainandrandom_stochastic_matrix(#102). Withsparse=Val(true), the matrix is built directly as aSparseMatrixCSCfrom the sampled(row, column, value)triplets, without materializing it densely. TheVal-typed flag keeps the methods type stable — tested with@inferred— which a plainBoolkeyword could not, while avoiding a matrix-type argument that the user would have to spell out.The existing behavior is unchanged, and the random number stream is preserved: for a given
rng, the dense and sparse formats sample the identical matrix, since the nonzero-position sampling is shared in_random_nonzero_indicesand consumes no random numbers whenk == n. This is tested by comparing the two formats at a fixed seed.A second commit renames the private generators in
benchmark/mc_tools.jl(added in #391) tomc_-prefixed names: they shadowed therandom_stochastic_matrixexport, and they intentionally remain local — they guarantee row sums within the constructor tolerance and irreducibility of the sparse chain, and keep the benchmark models independent ofmarkov/random_mc.jlchanges.Closes #102.
🤖 Generated with Claude Code (Claude Fable 5)