Repository navigation
Conversation
Subject ids are text whenever a data file labels subjects with anything other than plain integers, and Pmetrics ordered and grouped data by that text. Text sorts by bytes, so a 12-regimen simulation came out as 1, 10, 11, 12, 2, 3, ... PM_pta numbers its rows by regimen position, so row 4 described the regimen with id 12 rather than id 4. The PDI calculation and `prop_success == mean(success)` were correct throughout; only the association was wrong. `pm_id_rank()` (new, internal) returns an integer rank per id: numeric ids sort numerically, anything else keeps byte order, and equal ids share a rank. It replaces `id` as the sort key at every site that decides regimen order: - PM_data standardization, where `group_by(id, ...)` orders text ids as text - PM_sim, where `.id` was ranked with `dense_rank(id)` and two `arrange(.id, ...)` calls sorted the id string - PM_pta, where `dense_rank(id)` reported the same ranking `dense_rank(id)` in PM_pta came from #341 ("fix: mis-sorting during PTA"), which converted ids to ranks to fix a sorting problem; the rank of a text id is lexicographic, so ids of different lengths were not covered. This is an incomplete earlier fix rather than a later regression. Ids that are already numeric are unaffected, and ids such as "P01"/"P02" keep the order they had. The regression test builds 12 regimens with text ids and log-spaced doses spanning the target boundary, then compares each multi-regimen PM_pta row with PM_pta run on that regimen alone, so it tests the association without reimplementing the PDI calculation. Without this fix it fails with the reported pattern ("1" "10" "11" "12" "2" ...). Full testthat suite: no failures.
PM_pta identified a regimen by its position in the list of simulations: the id was replaced by a rank in `initialize()` and the rows were numbered by the loop index in `make()`. Nothing downstream could tell which regimen a result belonged to, so whenever the simulations came back in a different order than the data the rows described the wrong regimen. That happens for any id that is not a plain integer, because sorting text is byte order and "10" precedes "2". The ranking added in #391 orders the results so that this is less likely, but it does not restore the association: ids such as P1..P12 keep byte order, and a rank is not an identifier. This keeps the identifier with the result instead: - `initialize()` splits the simulated observations by id while preserving the order the ids first appear, and keeps the id itself as the name of each element rather than replacing it with a rank. This also fixes the `PM_sim_data` path (`PM_pta$new(simEx$data)`), which split the whole object instead of its observations and errored. - `make()` carries the id of each regimen into every result tibble, including `intersect`, and `summary()` reports it. `reg_num` stays the position of the regimen in `simdata` - the order in which its id first appears - which is what `start`, `end`, and `simlabels` are keyed on, and for user-supplied lists that repeat an id it stays positional. - Results saved before the id column existed fall back to the regimen number, which is the only identifier those rows were ever keyed on, so old objects still summarise and plot. The ranking is left in place for ordering: the order of the regimens is still user-visible through `reg_num` and `label`, and the order of the data is what PM_data and PM_sim already report. Nothing depends on it for correctness now. The regression test is kept and extended. It asserts the id column for numeric, character, and prefixed ids; compares each row with a single-regimen run for the id it reports, so the association is checked without depending on the order; and covers a shuffled dose assignment, a subset of regimens, a user-supplied list of regimens, and a saved object without the id column. The new assertions fail on both the base commit and on #391. Not changed, but worth knowing: the `postToUse[i]`/`toInclude[i]` pairing in PM_sim's posterior branch joins posteriors to subjects by position, so a posterior file listing ids in a different order than the data still attaches the wrong posterior. PM_pta's new id column does not fix that, but it makes such a mix-up visible in the results. The single-regimen paths for PM_post and PM_data (dataType 5-8) are broken before this change - `simdata` is nested one level too deep for `make()` to read it - and are left as they are. Full testthat suite: no failures.
A PTA result is now labeled with the id of the regimen it was calculated from, instead of the generic "Regimen n". The regimen is already reported in the `id` column, so the default label repeats the identifier rather than a position, and a plot legend or summary reads as the regimens the user asked for. Explicit `simlabels` are unchanged: they are still assigned positionally in `reg_num` order, and both length warnings are untouched. A regimen whose id cannot be recovered - an unnamed list of objects that carry no id column - keeps the generic "Regimen n" text rather than becoming NA, because `plot.PM_pta` builds its legend and axis labels from `unique(label)` and a missing label would break plotting. Docs for `simlabels`, the `label` field, and the plot legend are updated, and `man/` is regenerated. The regression test gains the label policy in both directions: the default equals the id for numeric, character, and prefixed ids and carries through `summary()`; supplied labels follow `reg_num` while the id still identifies the regimen; and an unrecoverable id falls back to the generic text and still plots. The legacy-object test now also resets the labels, so it emulates an object saved before either change. Full testthat suite: no failures.
mhovd
marked this pull request as draft
September 16, 2026 12:08
`private$make()` identified a regimen by index arithmetic: it nested a map over regimens and target types, then looked up `reg_ids[x]`, `simdata[[x]]`, `target[[y]]`, `success[[y]]` and `start[y]`, and rebuilt the name of the `pta_*` helper as a string in order to call it. Nothing about a result carried the regimen it came from. It now builds one row per regimen and target, with the regimen's data, id, start and end in the row, and maps over those rows with `purrr::pmap`, choosing the helper from a lookup keyed by the target type. `expand_grid()` is used rather than `crossing()` because it preserves the order of its inputs, so the rows stay regimen-major and then follow the order of the target types and values, which is the order results have always been reported in. The `-` sign handling, the progress bar, the sampled-target branch, the `this_type` split and the `intersect` assembly are unchanged. This exposed a bug in `start` and `end`: they were recycled to one value per regimen, then remapped with `start[x]` where `x` is the *target type* index, so the per-regimen values were discarded and every regimen silently got the first regimen's window. A vector `start` or `end` is now honoured per regimen, and a numeric `target_type` still overrides both with its own absolute value. `pm_id_rank()`'s fallback sorted with `sort()`, which follows the session collation, while dplyr's `arrange()` and `dense_rank()` use the C locale, so the order of the regimens and of `standard_data` depended on the user's locale. It now sorts with `method = "radix"` (byte order) and keeps `na.last = TRUE`. Simulating from posteriors paired `postToUse[i]` with `toInclude[i]` by position, so a posterior file listing its ids in a different order than the data attached the wrong posterior to each subject. The two are paired by id now, and a template with no posterior aborts with a clear message. `NPex$postMean` is empty, so this cannot be exercised from the shipped data. Verified with a parity harness against the previous commit: 26 of 31 fixtures produce identical `PM_pta$data`, and the 5 that differ are the per-regimen `start`/`end` cases, differing only in the intended way. Full testthat suite: no failures.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical posterior-pairing and legacy-loading issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR preserves regimen IDs through simulation and PTA results, preventing misassociation when regimen order differs.
Changes:
- Adds ID-based ordering, labels, summaries, and posterior pairing.
- Supports per-regimen travel windows and legacy data.
- Adds regression tests and documentation updates.
File summaries
| File | Summary |
|---|---|
tests/testthat/test-pm-pta-regimen-order.R |
Adds regression coverage for regimen ordering and compatibility. |
R/PM_sim.R |
Updates simulation ordering; posterior pairing still has a critical indexing issue (3 votes). |
R/PM_pta.R |
Propagates regimen IDs; legacy loading has a critical issue (2 votes) and empty-name fallback has a moderate issue (1 vote). |
R/PM_data.R |
Applies deterministic regimen ordering. |
R/id_order.R |
Adds shared identifier-ordering logic. |
man/summary.PM_pta.Rd |
Documents summary IDs. |
man/PM_pta.Rd |
Documents PTA ID and label behavior. |
man/plot.PM_pta.Rd |
Documents ID-based default legends. |
Review details
Files not reviewed (3)
- man/PM_pta.Rd: Generated file
- man/plot.PM_pta.Rd: Generated file
- man/summary.PM_pta.Rd: Generated file
Suppressed comments (1)
R/PM_pta.R:353
- Checking only the length of
names(simdata)treats empty element names as recoverable IDs. A partially named list of id-less regimens such asnames(simdata) = c("", "dose B")leaves the firstreg_idsentry as"", so it becomes an empty default label instead of the documentedRegimen 1fallback and can produce a blank plot legend. Only copy non-empty names into missing IDs.
if (anyNA(reg_ids) && length(names(simdata)) == n_reg) {
# a user-supplied list may name its elements with the regimen ids
reg_ids[is.na(reg_ids)] <- names(simdata)[is.na(reg_ids)]
}
- Files reviewed: 5/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 64.77% 65.75% +0.97%
==========================================
Files 52 53 +1
Lines 22964 23018 +54
==========================================
+ Hits 14876 15135 +259
+ Misses 8088 7883 -205
🚀 New features to boost your workflow:
|
`postToUse` was matched to the templates by id, but `getSimPrior()` consumes that value as a row index into `postMean` and as a list index into `postCov`, so the pairing was still positional. With posterior ids `c(10, 20)` and templates `c(20, 10)` the indices became `c(20, 10)` for a two-row frame: the wrong posterior, or out of bounds. Ids and positions coincide only when the posteriors happen to be numbered 1..n in the order they are stored, which is why the existing tests passed. The conflation is pre-existing; matching by id made it reachable for ids that are not 1..n. `match_posteriors()` returns the position of each template's posterior and aborts on duplicate posterior ids, which is what makes a single `match()` valid for both structures, or on a template with no posterior, naming the subjects. The parameter is carried as `post_pos` through `getSimPrior()` and both call sites. The count check it replaces reported the same situation less clearly. `postCov` is built by splitting the posterior on id, so it carries the ids as names, while `postMean` is ordered by a dplyr group; those orderings agree for ids that are numbers but need not for others. `posterior_cov()` uses the name when it is available and falls back to the position. Tested against the shipped `NPex$final`, which has a 20-row `postMean` and 20 `postCov` entries. With `nsim = 1` the posterior mean is drawn exactly, so the simulated parameters reveal which posterior each subject was given. Reversing the stored order, relabelling the ids to `10k`, and the helper's own shape (`c(20, 10)` against `c(10, 20)`) all pass here and all fail before this change: 3 failures and 2 errors. Full testthat suite: no failures.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (3)
- man/PM_pta.Rd: Generated file
- man/plot.PM_pta.Rd: Generated file
- man/summary.PM_pta.Rd: Generated file
Suppressed comments (3)
R/PM_pta.R:301
PM_pta$save()serializes the R6selfobject, so reading a normal saved PTA here returns aPM_pta, not aPM_pta_data.backfill_pta_id()only handles data lists/data frames and therefore returns that R6 object unchanged;private$populate()then stores the whole PTA as$data, so loading a legacy saved PTA still cannot backfillidand subsequent summaries fail. Unwrap the saved object's$databefore populating (and add a save/reload regression test).
pta <- backfill_pta_id(PM_upgrade(readRDS(simdata)))
R/PM_pta.R:245
- Please add a regression test that passes
simEx$datadirectly. The new branch fixes the documentedPM_sim_datainput, but the added PTA tests exercisePM_simobjects and raw regimen lists; a regression to the old$idaccess would therefore go unnoticed.
if (dataType == 1) { # PM_sim_data object
simdata <- split_by_regimen(simdata$obs) # split by id
R/id_order.R:26
- The numeric ordering path is skipped as soon as
unique_idscontains anNA:as.numeric(unique_ids)then has anNA, so IDs such asc(10, NA, 2)take the character/radix branch and order as10, 2, NAinstead of numeric2, 10, NA. Since this helper explicitly promises numeric ordering and missing IDs last, exclude missing values from the numeric check and append them after the numeric order.
numeric_ids <- suppressWarnings(as.numeric(unique_ids))
ordered <- if (all(!is.na(numeric_ids))) {
unique_ids[order(numeric_ids)]
} else {
# method = "radix" sorts in the C locale, the byte order dplyr's arrange()
# and dense_rank() use, so the order does not follow the session collation.
sort(unique_ids, na.last = TRUE, method = "radix")
- Files reviewed: 6/9 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+2755
to
+2758
| if (!is.null(names(postCov)) && key %in% names(postCov)) { | ||
| return(postCov[[key]]) | ||
| } | ||
| postCov[[post_pos]] |
`$save()` writes the whole `PM_pta` object, so `readRDS()` hands back an R6 object. `backfill_pta_id()` understands a `PM_pta_data` list or a data frame, so it returned that object untouched, and `private$populate()` then stored the whole loaded PTA as `self$data`. `PM_pta$new(<file>)` produced an object whose `$summary()` and `$plot()` failed with "wrong arguments for subsetting an environment", and the `id` backfill added for legacy objects never ran on that path, so the claim in the description only held for the `readRDS(f)$summary()` flow. The loaded object is now unwrapped to its `$data` before backfilling and populating, with a path kept for a bare `PM_pta_data` list. `PM_upgrade()` is applied once to whatever was loaded: it walks the data frames of a bare list and for an R6 object upgrades the list held in `$data`, so both shapes are covered and nothing is upgraded twice. Two tests cover the documented round trip and the legacy case, both loading through `PM_pta$new(<file>)`: the result list is populated, every table has an `id` column, `summary()` reports it, and `plot()` works. Both fail before this change with 2 failures and 2 errors. The legacy test's check that the `id` column really was stripped now looks at every table rather than at `$data`, which only exists for a single target type and made that assertion vacuous. Full testthat suite: no failures.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate issues remain, along with a requested end-to-end test for the PM_sim_data input path.
Review details
Files not reviewed (3)
- man/PM_pta.Rd: Generated file
- man/plot.PM_pta.Rd: Generated file
- man/summary.PM_pta.Rd: Generated file
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
R/PM_pta.R:652
regimen_id()treats the first row's subjectidas the regimen id even when the input is a singlePM_data/PM_postobject. Those branches build one regimen containing all subjects (lines 275–282), so a multi-subject input is now labeled and reported with an arbitrary subject id instead of falling back toRegimen 1, and the newidcolumn is not a regimen identifier. Only use an id when the source is known to be one simulated regimen (or otherwise preserve that provenance); aggregate data inputs should remain unrecoverable and use the generic label.
R/PM_pta.R:245
- This is the regression fix for the documented
PM_pta$new(simEx$data)(PM_sim_data) input, but the new tests exercisePM_simand raw regimen lists rather than this branch. Please add an end-to-end test that passes aPM_sim_dataobject directly and verifies that PTA construction succeeds and carries the regimen id, so this public input path cannot regress.
if (dataType == 1) { # PM_sim_data object
simdata <- split_by_regimen(simdata$obs) # split by id
R/id_order.R:22
- When numeric ids include a missing value,
all(!is.na(numeric_ids))is false and the fallback sorts every id as character. For example,pm_id_rank(c("10", NA, "2"))returns ranksc(1, 3, 2)instead of preserving numeric order with the missing id last, contrary to the helper's numeric-id contract.
numeric_ids <- suppressWarnings(as.numeric(unique_ids))
ordered <- if (all(!is.na(numeric_ids))) {
unique_ids[order(numeric_ids)]
- Files reviewed: 6/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Collaborator
Author
|
Discussed with @mnneely , to change |
mhovd
marked this pull request as draft
October 2, 2026 18:28
This branch has not been deployed
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.
PTA rows were keyed by position (
reg_num), so whenever the simulation returned regimens in a different order than the data, every result was associated with the wrong regimen. The engine returns theidcolumn as text, so this happened for text ids ("10" sorts before "2") and for plain numeric ids too. Results now carry the regimen they came from instead of relying on position.PM_ptalabels every result with theidof the regimen it was calculated from, including theintersectelement, andsummary()reports it.reg_numstays positional.labelis that id, falling back to"Regimen n"for a regimen whose id cannot be recovered. Explicitsimlabelsare still assigned inreg_numorder.private$make()builds one row per regimen × target and maps over those rows, so a regimen's id, data, start and end travel together, and thepta_*helper is chosen by name rather than rebuilt as a string.PM_pta$new(simEx$data)(aPM_sim_datainput), which previously errored.Supersedes #391, which fixed the same mis-association by ranking ids; that PR's regression test and ordering changes are carried here, and the id column is what makes the association correct.
Not breaking, but the output changes:
PM_pta$dataandsummary()gain anidcolumn, and the default label changes from"Regimen n"to the regimen id. Per-regimenstart/endare now honoured — previously only the first regimen's value was used — so results change for callers who passed a vector. No argument, class or method is removed. Objects saved before this change haveidbackfilled fromreg_num, the only identifier those rows ever had, both when summarising a loaded object and when loading it withPM_pta$new(<file>).