From 92c5e358295a78063ff824fc686b6dd4c162e736 Mon Sep 17 00:00:00 2001 From: "George G. Vega Yon" Date: Fri, 25 Sep 2026 19:26:14 -0600 Subject: [PATCH 1/3] 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 --- DESCRIPTION | 6 ++-- NEWS.md | 16 ++++++++++ R/RcppExports.R | 4 +++ inst/tinytest/test_formula_parser.R | 47 +++++++++++++++++++++++++++++ man/defm_terms.Rd | 4 +++ src/terms.cpp | 4 +++ 6 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 inst/tinytest/test_formula_parser.R diff --git a/DESCRIPTION b/DESCRIPTION index 05a730c..2390206 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,7 +1,7 @@ Package: defm Type: Package Title: Estimation and Simulation of Multi-Binary Response Models -Version: 0.2.2.0 +Version: 0.2.2.9000 Authors@R: c( person("George", "Vega Yon", role=c("aut", "cre"), email="g.vegayon@gmail.com", comment = c(ORCID = "0000-0002-3171-0844")), @@ -22,8 +22,7 @@ License: MIT + file LICENSE Encoding: UTF-8 LazyData: true Roxygen: list(markdown = TRUE) -RoxygenNote: 7.3.3 -LinkingTo: Rcpp, barry (>= 0.2.2) +LinkingTo: Rcpp, barry (>= 0.2.2.9000) Imports: Rcpp, stats @@ -34,3 +33,4 @@ Suggests: texreg, tinytest, barry Remotes: USCbiostats/barryr +Config/roxygen2/version: 8.0.0 diff --git a/NEWS.md b/NEWS.md index a5f11d7..d5a6f54 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,19 @@ +# defm 0.2.2.9000 (development version) + +* Requires `barry` (>= 0.2.2.9000), which fixes the `td_formula()` parser + (USCbiostats/barry#26): + - Covariate names containing `y` (e.g., `Day1`) were read as motif + terms, silently changing the model: `"{y0} > {y0} x Day1"` became + `Motif {a+}>{a+, b+} x Day1`. + - `"{...}x Covar"` (no space before `x`) silently dropped the covariate. + - Transitions such as `"{y0_1} > {y1}"`, with a current-time term on the + left-hand side, were silently turned into intercept motifs; they now + throw an error. + +* Also picks up the `barry` fix for wrong draws in `sim_defm()` caused by a + support-vs-array index confusion (USCbiostats/barry#25). + + # defm 0.2.2.0 * Requires `barry` (>= 0.2.2), which fixes a hash-collision bug that made diff --git a/R/RcppExports.R b/R/RcppExports.R index 5cc07d3..f7a2590 100644 --- a/R/RcppExports.R +++ b/R/RcppExports.R @@ -432,6 +432,10 @@ td_generic <- function(m, mat, covar = "") { #' - Intercept effect: `{...} x Hispanic` interacts with the Hispanic covar. #' - Transition effect: `{...} > {...} x Hispanic` Same. #' +#' The `x` must be followed by a space. Only the terms inside the curly +#' brackets are part of the motif, so covariate names such as `Day1` are +#' fine. +#' #' ## Intercept effects #' #' Intercept effects only involve a single set of curly brackets. Using the diff --git a/inst/tinytest/test_formula_parser.R b/inst/tinytest/test_formula_parser.R new file mode 100644 index 0000000..25e4943 --- /dev/null +++ b/inst/tinytest/test_formula_parser.R @@ -0,0 +1,47 @@ +# Regression tests for the formula parser (USCbiostats/barry#26): covariate +# names are not motif terms, `}x Covar` keeps the covariate, and the LHS of a +# transition cannot include the current time. +set.seed(1) +n <- 40 +id <- rep(1:10, each = 4) +Y <- matrix( + rbinom(n * 2, 1, .5), ncol = 2, + dimnames = list(NULL, c("a", "b")) + ) +X <- cbind(Day1 = runif(n), Female = rbinom(n, 1, .5)) + +new_model <- function() new_defm(id, Y, X, order = 1) + +# Covariate names containing y +m <- new_model() +td_formula(m, "{y0} > {y0} x Day1") +td_formula(m, "{y0} > {y1} x Day1") +td_formula(m, "{y0, 0y1} x Day1") +expect_equal( + names(m), + c( + "Motif {a+}>{a+} x Day1", + "Motif {a+}>{b+} x Day1", + "Motif {a+, b-} x Day1" + ) +) + +# The statistic is weighted by the covariate +init_defm(m) +S <- get_stats(m) +prev <- rbind(NA, Y[-n, ]) +prev[c(TRUE, id[-1] != id[-n]), ] <- NA +expect_equal( + unname(S[, 1]), + ifelse(is.na(prev[, 1]), NA, (prev[, 1] == 1 & Y[, 1] == 1) * X[, "Day1"]) +) + +# No space before `x` keeps the covariate +m <- new_model() +td_formula(m, "{y0} > {y1}x Female") +expect_equal(names(m), "Motif {a+}>{b+} x Female") + +# The LHS cannot include the current time +m <- new_model() +expect_error(td_formula(m, "{y0_1} > {y1}"), "LHS") +expect_error(td_formula(m, "{y0_5} > {y1}"), "out of range") diff --git a/man/defm_terms.Rd b/man/defm_terms.Rd index 50a26e2..ac586b7 100644 --- a/man/defm_terms.Rd +++ b/man/defm_terms.Rd @@ -91,6 +91,10 @@ for example: \item Intercept effect: \verb{\{...\} x Hispanic} interacts with the Hispanic covar. \item Transition effect: \verb{\{...\} > \{...\} x Hispanic} Same. } + +The \code{x} must be followed by a space. Only the terms inside the curly +brackets are part of the motif, so covariate names such as \code{Day1} are +fine. \subsection{Intercept effects}{ Intercept effects only involve a single set of curly brackets. Using the diff --git a/src/terms.cpp b/src/terms.cpp index 4d5aa22..d39d976 100644 --- a/src/terms.cpp +++ b/src/terms.cpp @@ -168,6 +168,10 @@ SEXP td_generic( //' - Intercept effect: `{...} x Hispanic` interacts with the Hispanic covar. //' - Transition effect: `{...} > {...} x Hispanic` Same. //' +//' The `x` must be followed by a space. Only the terms inside the curly +//' brackets are part of the motif, so covariate names such as `Day1` are +//' fine. +//' //' ## Intercept effects //' //' Intercept effects only involve a single set of curly brackets. Using the From 05053dc0750a61d195c3623ca1285c9ef8afba53 Mon Sep 17 00:00:00 2001 From: "George G. Vega Yon" Date: Fri, 25 Sep 2026 19:38:10 -0600 Subject: [PATCH 2/3] 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 --- DESCRIPTION | 2 +- NEWS.md | 7 ++++--- inst/tinytest/test_formula_parser.R | 6 ++++++ 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 2390206..ce31042 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -22,7 +22,7 @@ License: MIT + file LICENSE Encoding: UTF-8 LazyData: true Roxygen: list(markdown = TRUE) -LinkingTo: Rcpp, barry (>= 0.2.2.9000) +LinkingTo: Rcpp, barry (>= 0.2.2) Imports: Rcpp, stats diff --git a/NEWS.md b/NEWS.md index d5a6f54..abafc4e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,7 +1,8 @@ # defm 0.2.2.9000 (development version) -* Requires `barry` (>= 0.2.2.9000), which fixes the `td_formula()` parser - (USCbiostats/barry#26): +* Building against `barry` (>= 0.2.2.9000), currently available from GitHub + (USCbiostats/barryr), fixes the `td_formula()` parser (USCbiostats/barry#26). + The CRAN version of `barry` still has these bugs: - Covariate names containing `y` (e.g., `Day1`) were read as motif terms, silently changing the model: `"{y0} > {y0} x Day1"` became `Motif {a+}>{a+, b+} x Day1`. @@ -10,7 +11,7 @@ left-hand side, were silently turned into intercept motifs; they now throw an error. -* Also picks up the `barry` fix for wrong draws in `sim_defm()` caused by a +* The same `barry` version also fixes wrong draws in `sim_defm()` caused by a support-vs-array index confusion (USCbiostats/barry#25). diff --git a/inst/tinytest/test_formula_parser.R b/inst/tinytest/test_formula_parser.R index 25e4943..5c6c8e5 100644 --- a/inst/tinytest/test_formula_parser.R +++ b/inst/tinytest/test_formula_parser.R @@ -1,6 +1,12 @@ # Regression tests for the formula parser (USCbiostats/barry#26): covariate # names are not motif terms, `}x Covar` keeps the covariate, and the LHS of a # transition cannot include the current time. +# +# The fix lives in barry's headers (LinkingTo), so these tests only apply when +# defm was built against barry >= 0.2.2.9000. +if (utils::packageVersion("barry") < "0.2.2.9000") + exit_file("barry < 0.2.2.9000 does not include the formula parser fix.") + set.seed(1) n <- 40 id <- rep(1:10, each = 4) From 62e42178733ab382ee25a4a0280ec17111b6e2f3 Mon Sep 17 00:00:00 2001 From: "George G. Vega Yon" Date: Fri, 25 Sep 2026 19:42:51 -0600 Subject: [PATCH 3/3] 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 --- inst/tinytest/test_formula_parser.R | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/inst/tinytest/test_formula_parser.R b/inst/tinytest/test_formula_parser.R index 5c6c8e5..0d7c069 100644 --- a/inst/tinytest/test_formula_parser.R +++ b/inst/tinytest/test_formula_parser.R @@ -1,12 +1,6 @@ # Regression tests for the formula parser (USCbiostats/barry#26): covariate # names are not motif terms, `}x Covar` keeps the covariate, and the LHS of a # transition cannot include the current time. -# -# The fix lives in barry's headers (LinkingTo), so these tests only apply when -# defm was built against barry >= 0.2.2.9000. -if (utils::packageVersion("barry") < "0.2.2.9000") - exit_file("barry < 0.2.2.9000 does not include the formula parser fix.") - set.seed(1) n <- 40 id <- rep(1:10, each = 4) @@ -18,6 +12,18 @@ X <- cbind(Day1 = runif(n), Female = rbinom(n, 1, .5)) new_model <- function() new_defm(id, Y, X, order = 1) +# The fix lives in barry's headers (LinkingTo), i.e., it is compiled into +# defm. Instead of checking barry's version, we probe the parser defm was +# built with: the unfixed parser reads `y1` from `Day1` and throws +# "The term y1 shows more than once in the formula." +parser_fixed <- tryCatch({ + td_formula(new_model(), "{y0} > {y1} x Day1") + TRUE +}, error = function(e) FALSE) + +if (!parser_fixed) + exit_file("defm was built against a barry without the formula parser fix.") + # Covariate names containing y m <- new_model() td_formula(m, "{y0} > {y0} x Day1")