Skip to content

Fix defm_motif_parser reading covariate names as motif terms - #26

Merged
gvegayon merged 2 commits into
masterfrom
fix/defm-formula-parser
Sep 26, 2026
Merged

gvegayon merged 2 commits into
masterfrom
fix/defm-formula-parser

Conversation

@gvegayon

Copy link
Copy Markdown
Member

Problem

defm_motif_parser (used by counter_formula, i.e. defm::td_formula()) looked for motif variables with (0?)y([0-9]+)(_([0-9]+))? over the entire formula, including the x covariate (label) suffix. Any covariate name containing y<digit> changed the model silently:

Formula (order 1, Y = a, b) Before After
{y0} > {y0} x Day1 Motif {a+}>{a+, b+} x Day1 (wrong motif, no error) Motif {a+}>{a+} x Day1
{y0} x Day1 Motif {a+, b+} x Day1 a x Day1 intercept
{y0} x Day5 error: column out of range works
{y0} > {y1} x Day1 error: y1 shows more than once works

Related parsing bugs fixed along the way:

  • Dropped covariate: {y0} > {y1}x Female passed the syntax regex (\s*x\s*), but the extraction regex required \s+x\s+, so the interaction was silently ignored. The covariate name and label now come from the capture groups of the validating regex. x must be followed by whitespace ({y0} xFemale is now a syntax error rather than ambiguous). Trailing whitespace is no longer captured in the name.
  • LHS accepting current time: in two-group mode, only the RHS row was validated. {y0_1} > {y1} (order 1) was accepted and silently became the intercept motif {a+, b+}. Rows >= m_order in the LHS now throw.
  • Out-of-bounds write: with m_order == 1, explicit rows weren't range-checked ({y0_5} > {y1} indexed selected out of bounds). The range check now applies to both orders.

Tests

New tests/21-defm-formula-parser.cpp covers each case. It fails on master (first assertion) and passes with the fix. Full suite: 147 assertions in 23 test cases pass.

Follow-ups: a barryr header sync, then defm will require the new barry.

🤖 Generated with Claude Code

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>
@codecov

codecov Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.33%. Comparing base (8447b23) to head (5210868).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
+ Coverage   89.30%   89.33%   +0.03%     
==========================================
  Files          46       46              
  Lines        3170     3170              
==========================================
+ Hits         2831     2832       +1     
+ Misses        339      338       -1     

☔ 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.

Copilot review overview

🟡 Changes recommended

The checked-in standalone defm.hpp must be regenerated so its consumers receive the fix.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Fixes DEFM motif parsing so covariate names cannot alter motif terms.

Changes:

  • Separates motif and covariate parsing.
  • Adds row validation for transitions.
  • Adds regression coverage for parser edge cases.
File Description
include/​barry/​models/​defm/​formula.hpp Corrects parsing and row validation.
tests/​21-defm-formula-parser.cpp Adds regression tests.
tests/​main.cpp Registers the new tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

std::regex pattern_intercept(
std::string("\\{\\s*[01]?y[0-9]+(_[0-9]+)?(\\s*,\\s*[01]?y[0-9]+(_[0-9]+)?)*\\s*\\}") +
std::string("(\\s*x\\s*[^\\s]+([(].+[)])?\\s*)?")
"\\s*(" + pattern_group + ")" + pattern_covar

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.

Regenerated in 5210868 by running Rscript --vanilla barry-hpp.R. make barry.hpp reported the target as up to date because it has no prerequisites. defm.hpp now only scans the motif part. A standalone program using barry.hpp + defm.hpp gives 2 motif terms for {y0} > {y0} x Day1 and throws for {y0_1} > {y1}. barry.hpp also picks up the Model::sample fix from #25, which had not been regenerated either.

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
gvegayon merged commit 8a5907e into master Sep 26, 2026
8 of 12 checks passed
@gvegayon
gvegayon deleted the fix/defm-formula-parser branch September 26, 2026 17:52
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>
gvegayon added a commit to UofUEpiBio/defm that referenced this pull request Sep 26, 2026
* Require barry >= 0.2.2.9000 (formula parser fix) and add regression tests

barry's formula parser read motif variables from the whole td_formula()
string, including the covariate, so a covariate named e.g. `Day1` silently
added `y1` to the motif. It also dropped the covariate in `{...}x Covar`
and turned `{y0_1} > {y1}` into an intercept motif (USCbiostats/barry#26).

- Require barry (>= 0.2.2.9000), which includes the fix.
- Add inst/tinytest/test_formula_parser.R covering these cases, including
  a check of the covariate-weighted statistic against a hand computation.
- Document the covariate syntax in td_formula().
- Bump to 0.2.2.9000 and add NEWS.

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

* Depend on CRAN barry (>= 0.2.2); skip parser tests on older barry

Requiring barry (>= 0.2.2.9000) broke CI: neither CRAN nor barryr's main
branch provides it yet (USCbiostats/barryr#12 is still open). LinkingTo
goes back to barry (>= 0.2.2). The formula parser regression tests exit
early when the installed barry is older than 0.2.2.9000. They run
automatically once CI installs a barry with the fix.

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

* Guard parser tests by probing the compiled parser, not barry's version

The fix is compiled into defm from barry's headers (LinkingTo), so the
installed barry version may not match what defm was built with. The tests
now try `{y0} > {y1} x Day1`, which the unfixed parser rejects, and exit
early if it fails. This works regardless of how barry is versioned.

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

---------

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