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.
Collaborator
|
All CI seems to be failing, could you update the PR to fix it? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
==========================================
+ Coverage 64.77% 64.81% +0.03%
==========================================
Files 52 53 +1
Lines 22964 22979 +15
==========================================
+ Hits 14876 14894 +18
+ Misses 8088 8085 -3
🚀 New features to boost your workflow:
|
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.
Fixes the mis-association reported by Jim Rhodes: with multi-digit simulation ids in a multi-regimen
PM_sim,PM_ptarows described the wrong regimen.What happened
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. Sorting text is byte order, so regimens came out as
PM_ptanumbers its rows by regimen position (reg_num), so row 4 described id 12 rather than id 4 — exactly the pattern reported (rows 4, 5, 6 matching ids 12, 13, 14). Nothing in the PDI calculation was wrong, andprop_success == mean(success)held; only the association was.Three places decided that order:
PM_datastandardizationgroup_by(id, …)orders text ids as text; the standardized data is now arranged by natural id orderPM_simarrange(.id, …)sorted the id string in two places (getSimand the prediction-time expansion), and.idwas ranked withdense_rank(id)PM_ptadense_rank(id)on text ids ranks lexicographicallydense_rank(id)inPM_ptacame from #341 ("fix: mis-sorting during PTA"). That fix addressed a sorting problem by converting ids to ranks, but the rank of a text id is lexicographic, so it did not cover ids of different lengths. This is an incomplete fix rather than a later regression.The change
R/id_order.Radds one internal helper:It returns a rank, so equal ids stay together and it can be used directly inside
arrange()/group_by()pipelines. It replacesidas the sort key at every site that decides regimen order. Ids that are already numeric are unaffected; ids like"P01","P02"keep their existing order.Verification
A regression test builds 12 regimens with text ids (
"1"…"12") and log-spaced doses spanning the target boundary, then compares each multi-regimenPM_ptarow againstPM_ptarun on that regimen alone — so it tests the association without reimplementing the PDI calculation. It also asserts the simulated ids appear in numeric order, and guards against a vacuous pass if every regimen returned the same vector.Without the fix the test fails with the reported pattern:
With it, all 12 rows describe their own regimen. Full
testthatsuite: no failures.Scope
Only sites that decide regimen order were changed. Other
arrange(id, …)calls order rows within a subject (for example the crossing interpolation insidepta_time, and the NCA/validation paths), so they do not affect which regimen a result belongs to;pm_id_rank()is available if any of them needs the same treatment later.For analyses already run
PM_ptaresults are unaffected when ids are numeric, or when a single regimen is analysed at a time. For text ids with more than one regimen, the row order is the lexicographic order of the ids, so results should be re-associated by id (thelabelcolumn, when supplied, is assigned positionally and follows the same order). The reporter's v3.0.9 FDA analysis predates #341, which introduced this ranking path, and warrants the mapping check they describe.