Repository navigation
Expand range-notation missing codes on the v0.4 path (#58) - #59
Merged
Merged
Conversation
Four-task plan implementing the accepted v0.5 formula-evaluator ADR: formula spec type with construction-time sandbox validation, a post-baseline evaluate_mock_formulas() stage on the reserved formula sub-stream, mockFormula adapter wiring with the DerivedVar carve-out, and a stale-claims docs sweep. Additive: no seeded-output change, enforced by the #48 stage-index and pinned-value tests.
mock_formula()/mock_spec_formula() constructors populate the reserved formula/depends_on fields; validation rejects unparseable formulas, unknown referents, disallowed function symbols, and dependency cycles at construction, using the allow-list and topological ordering recovered from the v0.4 spike.
Review finding: the constructor's eager depends_on computation threw on syntax errors, bypassing error accumulation and the validate = FALSE opt-out that every sibling constructor honours. Parse failures now surface only through validation (making the per-variable parse branch live), with tests for the validate = FALSE path and multi-error accumulation. Plan code block synced.
…39) generate_mock_data_native() now skips type = "formula" variables (they carry no distribution to sample from) instead of rejecting the whole spec; a formula-only spec still returns the usual n-row, zero-column frame. The new exported evaluate_mock_formulas(data, spec, seed) computes each formula variable afterwards, in dependency order, under .with_mock_seed(stage = "formula"): evaluation runs in a restricted environment (data columns via as.list(), enclosing on a fixed base-function allow-list whose parent is emptyenv()) so a formula can never reach the caller's frame or globalenv, matching the sandbox contract from the ADR/plan. Also fixes a Task 1 review finding: .validate_formula_referents() and .order_formula_variables() re-parsed every formula and aborted on the first parse failure, which both duplicated the "could not be parsed" error and masked referent/cycle findings for other, parseable formulas in the same spec. Both now skip unparseable formulas (the per-variable validator already reports that error once) and keep checking the rest.
…in simstudy (#39) Task 2 review: the accumulate-and-skip helpers let evaluate_mock_formulas() silently drop unparseable formula variables under validate = FALSE; the evaluator now strict-validates its spec at entry like postprocess_mock_data(), so bad formulas fail loudly before generation. The simstudy baseline path gains the same formula-variable skip as the native backend, restoring the documented two-step workflow. Pins the length-mismatch, single-row scalar, and formula-only-spec edges.
Adapter (R/mock_spec_recodeflow.R):
- .details_mock_formula(details) reads the first non-blank mockFormula
among a variable's detail rows ("" if absent/blank).
- .recodeflow_to_spec_variable() constructs via mock_spec_formula() when a
non-blank mockFormula is present, ahead of the kind-dispatched
constructors, reusing the row's already-extracted rtype/missing-code/
garbage settings so postprocess behaves identically to other variables.
- The exclude_derived block now keeps DerivedVar::/Func:: rows that carry a
non-blank mockFormula (D6 carve-out); bare DerivedVar::/Func:: rows stay
excluded exactly as before.
Orchestrator (R/create_mock_data.R):
- Inverted the unsupported-variable check: type == "formula" is now
supported end-to-end. A stray non-empty `formula` field on a
non-formula variable still triggers the legacy fallback (defensive).
- Pipeline is now baseline -> evaluate_mock_formulas() -> postprocess;
comment updated to name all three seed sub-streams.
Diagnostics (R/mock_spec_postprocess.R):
- .postprocess_empty_diagnostics() adds derived = TRUE, formula, and
depends_on to each formula variable's diagnostics entry (D5).
Tests: extended tests/testthat/test-formula-vars.R with the Task 3 brief's
end-to-end mockFormula test, the D6 backward-compat exclusion test, the
formula-missing-codes postprocess test, and the formula-free no-op pin.
Full suite: FAIL 0 | WARN 35 | SKIP 12 | PASS 746 (738 baseline + 8 new,
no new WARN/SKIP). #48 seed-contract pins: 13/13 pass untouched.
devtools::check(): 0 errors, 0 warnings, 3 pre-existing notes.
Review finding: the exclude_derived carve-out read unfiltered variable_details while construction read databaseStart-filtered rows, so a variable with a mockFormula in one cycle but not another crashed generation for the cycle without it instead of staying excluded. Both decisions now use the same .filter_recodeflow_details() view, with a multi-database regression test pinning both directions.
) NEWS gains the Phase A feature entry (mockFormula column, mock_formula() API, sandboxed dependency-ordered evaluation; DerivedVar::/Func:: semantics unchanged). Stale-claims sweep: design-philosophy's deliberately-deferred section now records that algebraic mockFormula generation landed in v0.5 while Func:: dispatch and a general evaluator remain deferred; advanced-topics and reference-config qualify their blanket derived-variables-are-never-generated claims with the mockFormula carve-out; the formula-evaluator ADR status notes the implementation. Verified the migration vignette's fallback-conditions prose against a fresh render — no formula-fallback claim to update.
… formulas (#39) Whole-branch review Minors: warn when a formula produces a factor but rType is numeric (cut()/factor() with the default double rType silently returned level codes), and when a variable's detail rows carry conflicting mockFormula values. Adds a Phase B ADR caveat on internal- generic S3 dispatch reaching globalenv methods.
All four tasks of the #39 formula-evaluator plan shipped (commits 9dba1c0..779463d) but its step checkboxes were never ticked; tick them. Tick the post-v0.4.0 plan's Task 5 and Task 6 steps for the same reason. The seed-contract ADR status line still said its implementation plan was "not yet written" — it was written, executed, and merged via PR #48; the line now records that and points at follow-up #47.
CI previously ran only the pkgdown build (on main) and Copilot review; the test suite had never run anywhere but a developer laptop. The new R-CMD-check workflow runs on pushes to main/dev and PRs into them, using setup-r-dependencies so Suggests (including simstudy) are installed and the optional backend's tests execute rather than skip. simstudy was declared in Suggests but absent from renv.lock: it had never been installed locally, so snapshot(type = "all") never saw it, and every renv::restore() reproduced an environment where the simstudy backend could not be tested (10 tests skipped). The lockfile now carries simstudy 0.9.2 and its 11 dependencies; no existing package versions change. With it installed the suite runs FAIL 0 | WARN 35 | SKIP 3 | PASS 789. Also ignore the per-developer .superpowers/ scratch directory in both .gitignore and .Rbuildignore — it surfaced as a hidden-directory NOTE in a local R CMD check.
pkgdown::build_site() failed with three topics missing from the index: evaluate_mock_formulas, mock_formula and mock_spec_formula. Place the constructors beside their categorical/continuous/date siblings and the evaluator between the baseline generators and postprocess_mock_data, in pipeline order. The pkgdown workflow only runs on main, so this would have surfaced at release time otherwise.
…#50) simstudy::genData(0, def) builds its id table as data.table(x = 1:n), and 1:0 is c(1, 0) -- two rows -- so .evalWith() stops with "Both 'dtSim' and 'n' are set but are of different length: 2 != 0". This reproduces for both simstudy-eligible types (categorical and uniform continuous) on simstudy 0.9.2, regardless of R or data.table version. No draws happen at zero rows, so .generate_simstudy_baseline() now delegates n = 0 to the native generators, which already implement the typed zero-row contract from #35. A skip-guarded contract cell pins the simstudy output identical to the native output for a mixed spec, and the pre-existing test-simstudy-backend.R n = 0 test passes again. This was the one failure in the first R-CMD-check CI runs: the suite had never run with simstudy installed anywhere but a laptop with a temporary install, which is how #50 was first noticed.
The recodeflow adapter took recStart literally, so a missing-code row such
as "[997,999]" with NA::b (the usual CCHS and CHMS pattern for don't know,
refusal and not stated) became the code "[997,999]". A continuous variable
then failed in post-processing ("values that cannot be parsed"), and a
categorical variable silently wrote the literal string into the data: 100
of 500 rows of the CHMS sample's alc_11. All 15 variables the adapter
builds from the CHMS sample carried such a code.
A shared .expand_range_missing_codes() now expands an integer range into
its individual codes, splitting the row's proportion equally (the legacy
generators draw uniformly within the range), in both adapter paths: the
continuous/date .recodeflow_missing() and the categorical
extract_proportions() branch. A bracketed code that is not an integer range
fails with a message naming the variable, instead of passing through.
Also correct two errors in the minimal example's metadata, found the same
way: BMI's low garbage range had a stray parenthesis, and height's
low-garbage proportion was 1.00 instead of 0.01. The example's Gompertz
survival dates force the legacy generator, which tolerated all three
problems and hid them; with this change every other variable in the example
generates through the v0.4 pipeline, which #40 depends on.
DougManuel
added a commit
that referenced
this pull request
Sep 27, 2026
… 0.5.0 Brings #51, #52, #59 and #61 into release prep. NEWS: the dev entries for survival dates (#40), range missing codes (#58) and the minimal-example fixture corrections are filed under the 0.5.0 categories (breaking changes, new features, validation, bug fixes) plus new Deprecations and Known issues headings; every dev bullet is present, two in the forms release prep had already rewritten, which dev had not changed since the branches split. The heading carries the provisional release date. README mentions survival dates in the 0.5 summary and feature list. cran-comments corrects the local check record: the local check library lacks simstudy (3 notes, run with _R_CHECK_FORCE_SUGGESTS_=false), while CI installs all suggested packages and reports Status: OK.
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.
Summary
Fixes #58. Prerequisite for #40. Stacked on #52 (branched from its head, which carries #51); the diff shrinks to the single commit below once #51 and #52 merge. Merge order: #51 → #52 → this → #40 → #53.
The bug. The v0.4 recodeflow adapter took
recStartliterally, so a missing-code row such as[997,999]withNA::b, the standard CCHS/CHMS "don't know, refusal, not stated" pattern, became the code"[997,999]".values that cannot be parsed.alc_11had 100 of 500 rows equal to"[7, 9]". All 15 variables the adapter builds from the CHMS sample carried such a code.The fix. A shared
.expand_range_missing_codes()expands an integer range into its individual codes, splitting the row's proportion equally. That matches the legacy generators, which draw uniformly within the range (997/998/999 appear 117/115/112 times in 500 legacy rows). It is applied in both adapter paths: continuous/date (.recodeflow_missing()) and categorical (extract_proportions()branch). A bracketed code that isn't an integer range, such as[99.6,99.9]or[996], now fails with a message naming the variable, instead of passing through.Fixture corrections in
inst/extdata/minimal-example/variables.csv, made withread.csv/write.csvand verified to change exactly two cells:garbage_low_range:[-10;15])→[-10;15](stray parenthesis; one adapter test pinned the typo and is updated)garbage_low_prop:1.00→0.01(every row was low garbage; value confirmed by the maintainer)Why this blocks #40. The minimal example's Gompertz survival dates force the whole spec onto the legacy generator, which tolerated all three problems. Once #40 supports those dates, the example routes to v0.4. Without this PR it would fail there, in seven vignettes.
Verification (local, R 4.4.2 macOS arm64, simstudy installed)
test-range-missing-codes.R: 7 tests, written first and confirmed failing before the fix. They cover continuous and categorical expansion, the v0.4 path fails on range-notation missing codes such as [997,999] #58 reproduction throughcreate_mock_data(), no literal bracketed values in categorical output, non-integer and unparseable ranges erroring, and the minimal example's non-survival variables generating on the v0.4 path.errorcolumn.Review focus
[996]. It was never valid on the v0.4 path, but some metadata may use it for a single code.🤖 Generated with Claude Code