Add structured group columns to tidy_pool_obj() output - #53
Open
bailliem wants to merge 6 commits into
Open
Conversation
Add placeholder group columns (group_var, group_level_1, group_level_2) to tidy_pool_obj() output. Placeholders default to NA for group_var, lsm_type/alt for group_level_1 (lsm/trt rows), and NA/ref for group_level_2 (lsm/trt rows). Task 2 will replace placeholders with real names when enrichment inputs present.
Add optional vars/data arguments to tidy_pool_obj() that map ref/alt placeholders in group_level_1/2 to real group names (and group_var to the real column name) when exactly two group levels are present. Descriptions are upgraded accordingly. Falls back to placeholders with a cli_warn() when vars/data are missing, incomplete, or the group variable does not have exactly two levels. Uses as.factor() rather than factor() when counting group levels so unused factor levels are not silently dropped (factor() on an already-factor column re-derives levels from observed values only).
- Warn and fall back to placeholder group columns when the group variable is not a factor, instead of silently deriving ref/alt via alphabetical sort on a coerced character column (could reverse treatment labels). - Make the vars$group validity guard robust to NULL, non-character, and length != 1 values so malformed input warns instead of erroring. - Add regression tests for both fixes and for two previously-untested guard arms (vars$group absent from data; data supplied without vars). - Fix NEWS.md heading case and @return wording for group_var to match repo convention; regenerate man/tidy_pool_obj.Rd.
|
Thank you, @bailliem - I checked it briefly, and it looks good. I only have two cosmetic suggestions to consider:
grp_var = vars$group
grp_levels = levels(ADMI[[vars$group]])That said, there is a trade-off here: with this approach, users would need to do a bit more work, whereas currently they can simply supply I actually think the best solution would be to include the name and the levels of the grouping variable in the output of Many thanks! |
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
2 participants
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.
Closes #51
Summary
tidy_pool_obj()output is now programmer-friendly and structured, addressing the concerns raised in #51:group_var,group_level_1,group_level_2. All ten existing columns keep their names, order, and default-call values byte-identical —pool_to_ard(),efficacy_table(), and any external consumers are unaffected (regression-verified).vars/dataarguments: when supplied, the genericref/altlabels are replaced with real treatment-group names (group_var = "TRT", levels"Drug A"/"Placebo"; reference = first factor level, matching rbmi convention), and descriptions upgrade to the style proposed in the issue:"Difference: Drug A vs Placebo at Week 24".vars/datasupplied; missing/malformedvars$group; group variable with ≠ 2 levels; and a non-factor group column — the last prevents silently alphabetized (i.e. swapped) treatment labels.The
parameter_type"trt"→"contrast"vocabulary rename from the issue is deliberately deferred: it is a breaking change (efficacy_table(),plot_forest(),pool_to_ard(), and the tidy filters all switch on the current values) and is planned for the release that deprecates the legacy columns.Test plan
test-tidiers.R: placeholder semantics, byte-identical legacy columns, enrichment with real names, upgraded descriptions, and all four guard paths (including character group column and malformedvars$group).test-ard_conversion.R(300) andtest-efficacy_table.R(48) pass unchanged.devtools::check(): 0 errors, 0 warnings (environmental NOTEs only). Full suite: 1064 passing, 0 failures.