Skip to content

Fix support-vs-array index confusion in sampling and DEFM::simulate - #25

Merged
gvegayon merged 3 commits into
masterfrom
fix/sample-support-index
Jul 16, 2026
Merged

gvegayon merged 3 commits into
masterfrom
fix/sample-support-index

Conversation

@gvegayon

@gvegayon gvegayon commented Jul 15, 2026 •

Copy link
Copy Markdown
Member

Summary

Fixes a sampling bug where a simulated Markov/DEFM process would collapse the first time a conditioning state discovered mid-simulation was revisited. In the defm README "Example 2: A fun model" (a 10-state torus walk), the active state advances correctly through row 12 but breaks at row 13 and never recovers.

Root cause is a support-index / array-index mix-up in two places:

  1. Model::sample(const Array_Type &, params) — keys2support maps a hash key to a support index, but the reuse branch assigned i = locator->second (already a support index) and then computed a = arrays2support[i], mapping it through arrays2support a second time. So reusing a known conditioning state selected an unrelated support. The two "correct" rows in the README output were coincidences (a support index that happened to be a valid array index mapping back to the same support).

  2. DEFM::simulate — the mirror-image bug: the first process of each individual was sampled with sample(idx->at(model_num++), par), i.e. arrays2support[model_num] (a support index) passed to the sample(size_t, params) overload, which itself expects an array index and resolves it through arrays2support. This gives wrong first-row draws for the 2nd+ individuals in a multi-individual simulation.

Fix

Key on the support index directly in both places. The Array_Type overload now delegates probability computation to update_pset_probs(params, 1u, a), matching the existing sample(size_t, params) path and removing the duplicated recompute loop (which carried the same index confusion into its own likelihood(..., i, ...) call).

Tests

Adds tests/20-defm-simulate-torus.cpp: a deterministic 10-state torus walk (strong transition reward + strong ones penalty, so the outcome is RNG-independent and portable). It asserts every simulated row has exactly one active state advancing by one and wrapping y9→y0.

  • Fails on the old code (walk collapses at row 13).
  • Passes with this fix.
  • Full suite: 109 assertions in 21 test cases, all passing.

Verified against the defm README Example 2 (single individual) and a two-individual case where the second individual starts in a distinct state — both now walk the torus correctly for all rows.

🤖 Generated with Claude Code

`keys2support` maps a hash key to a SUPPORT index, but the reuse branch of
`Model::sample(const Array_Type &, params)` treated `locator->second` as an
array index and mapped it through `arrays2support` a second time. The first
time a conditioning state discovered mid-simulation was revisited, the
sampler drew from an unrelated support.

`DEFM::simulate` had the mirror-image bug: it passed
`arrays2support[model_num]` (a support index) to the `sample(size_t, params)`
overload, which expects an array index and resolves it through
`arrays2support` itself. This produced wrong first-row draws for the second
and later individuals in a multi-individual simulation.

Both are fixed by keying on the support index directly. The Array overload
now delegates probability computation to `update_pset_probs`, matching the
`sample(size_t, params)` path and removing the duplicated recompute loop
(which carried the same index confusion into its `likelihood` call).

Adds tests/19-defm-simulate-torus.cpp, a deterministic 10-state torus walk
that fails on the old code (the walk collapses at row 13) and passes now.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@gvegayon
gvegayon force-pushed the fix/sample-support-index branch from afc39dd to db46016 Compare July 15, 2026 22:31
@codecov

codecov Bot commented Jul 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.30%. Comparing base (de83fec) to head (83b3e7c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   89.04%   89.30%   +0.26%     
==========================================
  Files          46       46              
  Lines        3204     3170      -34     
==========================================
- Hits         2853     2831      -22     
+ Misses        351      339      -12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a support-index vs array-index mix-up that could cause DEFM/Markov simulation sampling to select the wrong support when reusing a previously seen conditioning state, and corrects the analogous indexing bug in DEFM::simulate for multi-individual runs. It also adds a regression test based on a deterministic 10-state torus walk.

Changes:

  • Fix Model::sample(const Array_Type&, params) to avoid remapping a support index through arrays2support and to reuse the update_pset_probs path.
  • Fix DEFM::simulate to pass an array index to sample(size_t, params) rather than a support index.
  • Add a deterministic torus-walk regression test and wire it into the test runner.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/main.cpp Adds the new DEFM torus simulation regression test to the test aggregator.
tests/20-defm-simulate-torus.cpp New regression test validating the simulated state advances deterministically around a 10-state torus.
include/barry/models/defm/defm-meat.hpp Fixes DEFM simulation to sample using the correct index type for the first process of each individual.
include/barry/model-meat.hpp Fixes support vs array index handling in Model::sample(Array_Type, params) and simplifies probability computation by delegating to update_pset_probs.

Comment on lines +1643 to +1646
// Making sure the powerset probabilities for this support are current
// before sampling from them (mirrors the sample(size_t, params) path).
if (!first_calc_done[a] || !vec_equal_approx(params, params_last[a]))
update_pset_probs(params, 1u, static_cast<int>(a));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — addressed in 83b3e7c. Added a pset_probs.empty() check to the guard, matching sample(size_t, params):

if (pset_probs.empty() ||
    !first_calc_done[a] ||
    !vec_equal_approx(params, params_last[a]))
    update_pset_probs(params, 1u, static_cast<int>(a));

init() adds arrays without sizing pset_probs, and a prior likelihood() call sets first_calc_done/params_last without ever filling it, so this guards both. Note there is a deeper, pre-existing cache-coherence limitation shared with sample(size_t, params): if likelihood() populated params_last for a second support and pset_probs was sized by a first support, a same-params reuse of that second support could still read an unfilled slice (e.g. defm_mle then sim_defm on the same object). That's out of scope here and would want a dedicated per-support 'pset-probs-valid' flag — happy to open a follow-up.

Comment thread tests/20-defm-simulate-torus.cpp Outdated
Comment on lines +22 to +33
const size_t n = 20u;
const size_t ny = 10u;
const size_t nx = 1u;

std::vector< int > id(n, 1);
std::vector< int > y(n * ny, 0);
std::vector< double > x(n * nx, 0.0);

// Column-major storage; the walk starts with y0 active.
y[0] = 1;

DEFM model(&id[0], &y[0], &x[0], n, ny, nx, 1u, true, true);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 83b3e7c. The test now simulates two individuals with different starting states (starts = {0, 3}) and asserts each one's walk advances by one from its own start. The second individual (starting at y3) exercises the DEFM::simulate first-row fix specifically: on the old code its walk collapses to all-zeros from its first sampled step. Confirmed the extended test fails against the pre-fix headers (Individual 2, row 3: expected single active state y5, got 0 active) and passes with the fix.

- Model::sample(Array&): add a pset_probs.empty() check to the update guard,
  matching sample(size_t, params). init() adds arrays without sizing
  pset_probs, and a prior likelihood() call sets first_calc_done/params_last
  without filling it, so the cache guard alone could index an empty buffer.

- tests/20-defm-simulate-torus.cpp: simulate two individuals with different
  starting states. The second individual (starting at y3) exercises the
  DEFM::simulate first-row fix; on the old code its walk collapses to
  all-zeros from its first sampled step.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gvegayon added a commit to USCbiostats/barryr that referenced this pull request Jul 15, 2026
Syncs the follow-up from USCbiostats/barry#25: add a pset_probs.empty()
check to the update guard, matching sample(size_t, params). init() adds
arrays without sizing pset_probs and a prior likelihood() call sets
first_calc_done/params_last without filling it, so the cache guard alone
could index into an empty buffer. Header stays byte-identical to barry.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@gvegayon
gvegayon merged commit 8447b23 into master Jul 16, 2026
8 of 12 checks passed
@gvegayon
gvegayon deleted the fix/sample-support-index branch July 16, 2026 00:05
gvegayon added a commit to USCbiostats/barryr that referenced this pull request Jul 16, 2026
…ling (#9)

* Sync headers from barry: fix support-vs-array index confusion in sampling

Mirrors USCbiostats/barry#25. `keys2support` maps a hash key to a SUPPORT
index, but the reuse branch of Model::sample(const Array_Type &, params)
treated locator->second as an array index and mapped it through
arrays2support a second time; the first time a mid-simulation conditioning
state was revisited, the sampler drew from an unrelated support (e.g. the
defm "torus" example collapses at row 13). DEFM::simulate had the
mirror-image bug on the first process of each individual.

Both are keyed on the support index directly; the Array overload now
delegates to update_pset_probs, matching the sample(size_t, params) path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Address review: guard empty pset_probs in Model::sample(Array&)

Syncs the follow-up from USCbiostats/barry#25: add a pset_probs.empty()
check to the update guard, matching sample(size_t, params). init() adds
arrays without sizing pset_probs and a prior likelihood() call sets
first_calc_done/params_last without filling it, so the cache guard alone
could index into an empty buffer. Header stays byte-identical to barry.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
gvegayon added a commit that referenced this pull request Sep 26, 2026
Runs barry-hpp.R so the standalone headers include the formula parser fix.
barry.hpp also picks up the Model::sample fix from #25, which had not been
regenerated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gvegayon added a commit that referenced this pull request Sep 26, 2026
* Fix defm_motif_parser reading covariate names as motif terms

The variable regex `(0?)y([0-9]+)(_([0-9]+))?` was run over the whole
formula, including the `x covariate (label)` suffix. A covariate such as
`Day1` silently added `y1` to the motif (`{y0} > {y0} x Day1` became
`{a+}>{a+, b+} x Day1`) or produced misleading errors (`{y0} x Day5`:
"column out of range").

Variables are now only read from the bracketed motif part, which is
captured by the same regex that validates the syntax. This also fixes:

- `{y0} > {y1}x Female` passed the syntax check, but the separate
  covariate regex required whitespace before `x`, so the interaction was
  silently dropped. The covariate is now taken from the validating match;
  `x` must be followed by whitespace (`{y0} xFemale` is a syntax error).
- Trailing whitespace is no longer part of the covariate name.
- Two-group transitions accepted rows >= m_order in the LHS, e.g.
  `{y0_1} > {y1}` (order 1) silently became the intercept motif
  `{a+, b+}`. This now throws.
- With m_order == 1, explicit rows were not range-checked (`{y0_5} > {y1}`),
  indexing out of bounds. This now throws.

Adds tests/21-defm-formula-parser.cpp covering all cases.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Regenerate single-file headers (barry.hpp, defm.hpp)

Runs barry-hpp.R so the standalone headers include the formula parser fix.
barry.hpp also picks up the Model::sample fix from #25, which had not been
regenerated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
gvegayon added a commit to USCbiostats/barryr that referenced this pull request Sep 26, 2026
Syncs inst/include/barry/models/defm/formula.hpp with USCbiostats/barry#26.
Motif variables were read from the whole formula, so a covariate named
e.g. `Day1` silently added `y1` to the motif. Also fixes `{...}x Covar`
dropping the covariate, current-time rows accepted on the LHS of a
transition, and unchecked rows with Markov order 1.

Bumps the version to 0.2.2.9000 so dependents (defm) can require it, and
adds NEWS entries for this fix and the unreleased USCbiostats/barry#25.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants