Seed micro transition cells by identity, not by iteration position - #1815
Open
alanlujan91 wants to merge 2 commits into
Open
Seed micro transition cells by identity, not by iteration position#1815alanlujan91 wants to merge 2 commits into
alanlujan91 wants to merge 2 commits into
Conversation
get_micro_markov_states drew each cell's MarkovProcess seed from self.RNG inside the loop over cells. The number and order of non-empty cells depends on the realized macro path, so a cell's seed was set by its position among populated cells rather than by which cell it is. That defeats a guarantee MarkovProcess.draw goes to some trouble to provide. It spawns one sub-RNG per source state from a single parent draw, precisely so that a counterfactual altering one transition row leaves the other rows' permutations alone. The isolation was being undone one level up: perturbing a few agents shifted every later cell's seed and redrew micro states for agents the perturbation never touched. Measured on the 400-agent general-format fixture: adding 100 agents in a previously empty macro pair changed the micro state of 140 agents in untouched cells. An independent measurement on a larger population put it at 15.5%. Seeds now come from _cell_seed, which derives them from the cell's own (macro_prev, macro_next, micro_prev). _micro_transition_cells previously documented its iteration order as load-bearing, which was true and is the thing that was wrong; that note is now corrected. The regression test rejects TWO wrong versions, which is the point of how it is written: - the original, 140 of 400 agents changed; - the tempting hoist-only fix that pre-draws seeds above the loop but indexes them by position, 150 of 400. It perturbs which (macro_prev, macro_next) pairs are realized rather than AgentCount. Varying AgentCount changes cell populations but not the cell list, so hoist-only passes that weaker version while leaving the defect in place. This changes simulated values wherever markov_shuffle=True, which is why it is separate from the review-fixes branch. Quota-exact transition counts are seed-invariant and unchanged, so the existing count assertions (9025 / 475 / 250 / 250) still hold. Full suite 871 passed.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a common-random-numbers (CRN) defect in HARK’s hierarchical Markov micro-state transition drawing when markov_shuffle=True, ensuring that a transition cell’s RNG seed depends on the cell’s identity rather than on the iteration position of occupied cells (which can change across scenarios due to different realized macro paths).
Changes:
- Adds
_cell_seed(...)to deterministically derive a per-cellMarkovProcessseed from(macro_prev, macro_next, micro_prev)plus a single per-call entropy draw. - Updates
AggIndMrkvConsumerType.get_micro_markov_statesto draw onebase_entropyper call and seed each transition cell via_cell_seed(...)rather than consumingself.RNGinside the per-cell loop. - Adds a regression test that verifies “untouched” cells retain identical draws when previously-empty macro transition pairs become occupied; updates the changelog to note the (intentional) simulated-value change under
markov_shuffle=True.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
HARK/ConsumptionSaving/ConsAggIndMarkovModel.py |
Implements identity-based per-cell seeding for markov_shuffle=True to preserve CRN isolation across scenario comparisons. |
tests/ConsumptionSaving/test_ConsAggIndMarkovModel.py |
Adds a regression test ensuring micro draws in unaffected cells do not change when unrelated cells become occupied. |
docs/CHANGELOG.md |
Documents the behavioral change and explains why identity-based cell seeding is required (and why simple “hoisting” is insufficient). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Stacked on #1814, which is where the rest of the post-merge review fixes live. Separate because this one changes simulated values wherever
markov_shuffle=True, and everything in #1814 is either a bug fix with a rejection test or proven bit-identical. Retarget tomainonce #1814 lands.The defect
AggIndMrkvConsumerType.get_micro_markov_statesdrew each transition cell'sMarkovProcessseed fromself.RNGinside the loop over cells. The number and order of non-empty cells depends on the realized macro path, so a cell's seed came from its position among populated cells rather than from which cell it is.That defeats a property
MarkovProcess.drawgoes to some trouble to provide. It spawns one sub-RNG per source state from a single parent draw, and its docstring calls this "a correctness requirement for common random numbers in scenario comparisons", precisely so a counterfactual altering one transition row leaves the other rows' permutations alone. The isolation was being undone one level up._micro_transition_cellsdocumented the iteration order as load-bearing. That was accurate, and it is the thing that was wrong: order-dependence is exactly what breaks CRN. The note is corrected.Measurement
On the existing 400-agent general-format fixture, adding 100 agents in a previously empty macro pair changed the micro state of 140 agents in untouched cells whose transition rows and agent sets were identical across the two runs. An independent measurement on a larger population put the contamination at 15.5%.
Why the test is written the way it is
It perturbs which
(macro_prev, macro_next)pairs are realized, notAgentCount. That distinction is the whole test:Hoisting is the tempting fix and it does not work: drawing one seed per occupied cell still indexes by position. A test that varied only
AgentCountwould pass the hoist-only version while leaving the defect in place, because changing populations does not change the cell list.Blast radius
Simulated values move wherever
markov_shuffle=True. The default path is untouched. Quota-exact transition counts are seed-invariant, so the existing assertions (9025 / 475 / 250 / 250) still hold unchanged.Full suite: 871 passed, exit 0.