Conversation
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.
The default PH-test
ranktransform isnp.cumsum(events). This ranks positions in the fitted model's stored row order rather than event times. It gives tied events different ranks and, for stratified fits, accumulates through strata sorted by their labels. Consequently, the diagnostic can change without changing the fitted model or survival data.Rank observed event times directly, assigning average ranks to ties. This preserves the existing event-rank convention for chronologically ordered, untied observations; it does not change the PH-test approximation or attempt to match modern R
cox.zph.Reproduction on 2,000 balanced synthetic observations: five event times, each with the same 400 covariate values. The fitted coefficient is exactly 0 and its SE is 0.01931656 in both row orders. Sorting the covariate within tied times gives rank-test p=3.745e-19; shuffling the same rows gives p=0.79865.
check_assumptions()consequently flags the sorted representation but not the shuffled one. With the correction, both rank-test p-values are approximately 1. The identity-transform control is approximately 1 throughout.A separate check uses 227 complete rows from
load_lung(), fits age and sex with strata defined by ph.ecog, and evaluates all 24 renamings of the four strata. The original age rank-test p-value ranges from 0.08725 to 0.61962, with unchanged coefficients. The correction gives 0.94072455 across all renamings, within floating-point tolerance. This is a representation-invariance check on released data, not a claim about the original study's conclusions.Validation on base
7a8fc34a013ecd79fa405017b89e1697c1cc6e17: five added tests cover unordered/tied times, array/Series inputs, censor exclusion, the untied control, row permutation and stratum relabeling. Before: four fail, one passes. The complete existing statistics test file plus these cases gives 47 passed. Black 22.8.0 formatting passes. The full package suite was not run. Released 0.30.3 also reproduces the defect.Related: #997 discusses discrepancies and suspected tie handling; this PR supplies a concrete order-invariance reproduction and a narrowly scoped correction. Prepared with AI assistance. No independent scientific review or changed published conclusion is claimed. The public audit includes the reproduction scripts, separate patches, environment records and before/after execution logs in a downloadable evidence archive.