Repository navigation
Fix td_formula covariate parsing (tests + docs; fix via barry#26) - #12
Merged
Merged
Conversation
…ests
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>
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>
|
Thank you for your contribution @gvegayon 🚀! The pkgdown site preview is ready for review 👉 Download here 👈! |
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>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #12 +/- ##
==========================================
+ Coverage 81.37% 81.45% +0.08%
==========================================
Files 8 9 +1
Lines 451 453 +2
==========================================
+ Hits 367 369 +2
Misses 84 84 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
What
LinkingTo: barry (>= 0.2.2)(the CRAN version) so CI can resolve dependencies. The motif formula parser fix (Fix defm_motif_parser reading covariate names as motif terms USCbiostats/barry#26, synced in Sync headers from barry: fix defm formula parser covariate handling USCbiostats/barryr#12) applies when defm is built against barry >= 0.2.2.9000.inst/tinytest/test_formula_parser.Rwith regression tests.td_formula()(roxygen + regeneratedman/defm_terms.Rd).Why
td_formula()could build a different model from the one written, with no error:{y0} > {y0} x Day1Motif {a+}>{a+, b+} x Day1Motif {a+}>{a+} x Day1{y0} > {y1}x FemaleMotif {a+}>{b+}(covariate dropped)Motif {a+}>{b+} x Female{y0_1} > {y1}Motif {a+, b+}(not a transition){y0} x Day5The root cause: barry looked for
y<col>tokens across the whole formula, including the covariate name.Reviewer notes
test_formula_parser.R) first check which parser defm was compiled with, by trying{y0} > {y1} x Day1(the old parser throws on it). If it fails they callexit_file(). This avoids depending on the installed barry version, which may differ from the headers defm was built against. Once Fix defm_motif_parser reading covariate names as motif terms USCbiostats/barry#26 and Sync headers from barry: fix defm formula parser covariate handling USCbiostats/barryr#12 are merged, CI installs barryr from GitHub (Remotes) and the tests run. I checked both paths locally: CRAN barry 0.2.2 gives 25/25 with the parser tests skipped, and barry 0.2.2.9000 gives 30/30.LinkingTotobarry (>= 0.2.3)once that is on CRAN."{...} xCovar"(no whitespace afterx) is now a syntax error instead of being ambiguous.y<digit>in its name, or}x, were estimating a different motif than intended and should be re-run.tinytest::test_package("defm")gives 30/30 passing against the barryr branch.🤖 Generated with Claude Code