Fix add_pvalue() matching Gray-test p-value to plotted outcome by name (#277) - #278
Merged
Merged
Conversation
#277) For competing-risks fits, add_pvalue() derived the outcome position from the row order of tidycmprsk::tidy() and reused that integer to index the wide p.value_* columns from tidycmprsk::glance(). Those two orderings are independent, so when they diverged the reported p-value belonged to a different competing event. The chosen outcome is now matched by name against glance()'s own outcome_* columns, and the paired p.value_* column is pulled. Adds a regression test using a fit where tidy() and glance() orderings differ. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Documents package architecture (add-on functions as ggplot2 layers via ggplot_add.* S3 methods), dependency handling, cli messaging convention, roxygen/testthat/vdiffr practices, the devtools workflow, and a pre-PR checklist. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Thanks for report @zhaohongxin0 ! Can you take a look at this PR, please? I think it should address the issue |
|
Thank you for the quick fix. I tested the PR head ( The plotted outcome now receives the correct Gray-test p-value:
Both values exactly matched the corresponding outcome-specific values from The fix works as intended in my external validation. Thank you again. |
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.
What
Fixes #277 — for competing-risks fits,
add_pvalue()could report the Gray-test p-value for the wrong competing event.Why
In
update_add_pvalue()(R/add_pvalue.R), the outcome position was derived from the row order oftidycmprsk::tidy(survfit)$outcomeand then reused to index the widep.value_*columns produced bytidycmprsk::glance(). Those two orderings are independent (tidy orders by first appearance of the label; glance numbers columns by failcode), so when they diverged the reported p-value belonged to a different outcome than the one plotted.Reproduced by re-leveling
death_crso the factor order differs from the failcode order: choosing"death from cancer"(tidy position 1) wrongly pulledp.value_1(0.766, belonging to"death other causes") instead of the correctp.value_2(0.159).How
The chosen outcome is now matched by name against
glance()'s ownoutcome_*columns, and the pairedp.value_*column is pulled. This removes any dependency ontidy()ordering. The now-unusedtidycmprsk::tidy()call and position calculation in the single-outcome-check block were removed; the multi-outcome abort is unchanged.Tests
tests/testthat/test-add_pvalue.Rusing a fit wheretidy()andglance()orderings differ. It asserts each plotted outcome reports its own p-value, that the two outcomes' values are not swapped, and includes a sanity check that the orderings genuinely differ (so the test actually exercises the bug).Reviewer notes
glance()outcome_*value exactly; the existing single-outcome guard andggcuminc()outcome validation keep the no-match path unreachable in normal use.NEWS.mdupdated with a bullet under the development version.🤖 Generated with Claude Code