Full support for shape-constrained PAMMs via scam (closes #286)#290
Closed
fabian-s wants to merge 2 commits into
Closed
Full support for shape-constrained PAMMs via scam (closes #286)#290fabian-s wants to merge 2 commits into
fabian-s wants to merge 2 commits into
Conversation
Centralize model-class handling in three internal generics (get_coefs, get_Vp, sample_coefs in R/model-helpers.R) and use them together with the existing make_X generic throughout the post-processing workflow (add_hazard, add_cumu_hazard, add_surv_prob, add_term, add_cif, add_trans_prob, get_cumu_coef, get_cumu_eff, compute_cumu_diff). For scam fits this fixes the previously incorrect use of the unconstrained-scale coefficients/covariance (coef()/$Vp): predictions and all three CI types (default, delta, sim) are now based on the re-parametrized coefficients ($coefficients.t) and their covariance ($Vp.t), matching predict.scam(se.fit = TRUE). Simulation-based CIs draw from the corresponding normal approximation, the same one underlying scam's own standard errors (the exact posterior of the constrained coefficients is not Gaussian; sampling on the unconstrained scale is numerically unusable because coefficients shrunk to the constraint boundary have near-degenerate log-scale variances). Also: * pamm() gains engine = "scam" * tidy_fixed.scam, get_cumu_coef.scam methods; get_plotinfo accepts scam objects (tidy_smooth, tidy_re, gg_* work for scam fits) * relax get_hazard/get_cumu_hazard/get_surv_prob class assertions to accept scam * new article vignettes/shape-constraints.Rmd showcasing monotone baseline hazards and covariate effects, registered in _pkgdown.yml * tests: tests/testthat/test-scam.R covering hazard/se correctness against predict.scam, monotonicity, all CI types, grouped cumulative hazards, add_term, CIF, cumulative coefficients, pamm engine, predictSurvProb, tidiers and plots https://claude.ai/code/session_011bPCsxyWjbK9TzWA9M7ceH
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends pammtools’ post-processing/prediction workflow to correctly support shape-constrained additive models fit with scam::scam(), ensuring hazards/SEs/CIs are computed on the re-parameterized coefficient scale used by predict.scam(type = "lpmatrix").
Changes:
- Introduces internal helpers (
get_coefs(),get_Vp(),sample_coefs()) and routes downstream computations through the consistent(X, coefs, V)triplet to fix previously silent scale mismatches forscam. - Adds
pamm(engine = "scam"), plustidy_fixed.scam,get_cumu_coef.scam, and extends plotting/tidying support viaget_plotinfo()acceptingscamobjects. - Adds a new vignette (
shape-constraints) and a comprehensivetest-scam.Rsuite covering hazard/SE correctness, monotonicity, CI types, CIF, cumulative effects, andpec::predictSurvProb()compatibility.
Reviewed changes
Copilot reviewed 11 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
vignettes/shape-constraints.Rmd |
New article demonstrating monotone baseline/covariate effects with scam and notes on CI computation. |
tests/testthat/test-scam.R |
New test coverage validating correctness vs predict.scam(), monotonicity, CI modes, CIF, tidiers, plotting, and pamm(engine="scam"). |
R/tidiers.R |
Adds tidy_fixed.scam; updates get_plotinfo() to accept scam objects for tidiers/plot helpers. |
R/pammfit.R |
Adds engine = "scam" and resolves engine functions via namespaces (mgcv/scam). |
R/model-helpers.R |
New internal generics for coefficient/covariance extraction and coefficient simulation, enabling correct scam handling. |
R/cumulative-effect.R |
Switches cumulative effect computations to get_coefs()/get_Vp()/make_X(). |
R/cumulative-coefficient.R |
Adds get_cumu_coef.scam and routes simulation/point estimates through the new helpers. |
R/add-functions.R |
Refactors hazard/CI/CIF/transition-CI code paths to use get_coefs()/get_Vp()/sample_coefs() and expands class acceptance to scam. |
NEWS.md |
Documents the new full scam support and links the new article. |
NAMESPACE |
Registers new S3 methods and updates imports. |
man/*.Rd |
Updates/creates documentation entries for the new methods/helpers and updated behavior. |
_pkgdown.yml |
Registers the new vignette in articles and navbar. |
Files not reviewed (10)
- man/add_hazard.Rd: Language not supported
- man/add_term.Rd: Language not supported
- man/compute_cumu_diff.Rd: Language not supported
- man/cumulative_coefficient.Rd: Language not supported
- man/get_Vp.Rd: Language not supported
- man/get_coefs.Rd: Language not supported
- man/get_plotinfo.Rd: Language not supported
- man/pamm.Rd: Language not supported
- man/sample_coefs.Rd: Language not supported
- man/tidy_fixed.Rd: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
There was a problem hiding this comment.
I think we can consolidate helpers.R and model-helpers.R? Or was this a conscientous choice?
Collaborator
Author
There was a problem hiding this comment.
conscientious? maybe.
conscious? not sure - is claude conscious...? ;)
seems fine to me to leave these in their own file separate from the other helpers IMO
Collaborator
There was a problem hiding this comment.
Alright. Fine for me. conscious* Difficult word.
Collaborator
Author
|
see #292 |
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.
Closes #286.
Adds full support for shape-constrained additive models fit with
scam::scam()across the entirepammtoolspost-processing workflow, with a new article showcasing the functionality.What was broken
pammtoolsalready had partial scam handling (make_X.scam, a fewinherits(object, "scam")checks), but it was silently incorrect: forscamobjects,coef()and$Vpare on the underlying unconstrained scale, while the lpmatrix returned bypredict.scam(type = "lpmatrix")multiplies the re-parametrized (partially exponentiated) coefficient vector$coefficients.t, whose covariance is$Vp.t. SoX %*% coef(object)produced wrong hazards (and wrong SEs) for any model with constrained terms.Changes
New internal generics
get_coefs(),get_Vp(),sample_coefs()(R/model-helpers.R) abstract the(X, coefs, V)triplet next to the existingmake_X()generic. All consumers —add_hazard(),add_cumu_hazard(),add_surv_prob(),add_term(),add_cif(),add_trans_prob()/add_trans_ci(),get_cumu_coef(),get_cumu_eff(),compute_cumu_diff()— now go through them, which also removed the scatteredis_gambranching and hard-codedpredict.gam/$Vpcalls.glm/gam/bam/pammbehavior is unchanged.Confidence intervals: all three
ci_types ("default","delta","sim") work for scam fits. They are based on the normal approximation for the re-parametrized coefficients ($coefficients.t,$Vp.t) — the same approximation underlyingscam's own standard errors. (Sampling on the unconstrained scale and exponentiating per$p.identwould respect the constraints draw-by-draw, but is numerically unusable: coefficients shrunk to the constraint boundary have near-degenerate log-scale variances, soexp()of the draws overflows. Documented in the article.)pamm()gainsengine = "scam"(engine lookup now resolvesgam/bam/scamto the proper namespaces instead of relying on attached packages).tidy_fixed.scamandget_cumu_coef.scammethods;get_plotinfo()accepts scam objects, sotidy_smooth(),tidy_re()and thegg_*helpers work for scam fits;pec::predictSurvProb()works forpamm(engine = "scam")fits.Class assertions in
get_hazard()/get_cumu_hazard()/get_surv_prob()relaxed to accept scam objects (robust also for older scam versions whose objects did not inherit fromglm).New article
vignettes/shape-constraints.Rmd("Shape-constrained effects (scam)", registered in_pkgdown.yml): monotone-decreasing baseline hazard (bs = "mpd") vs. unconstrained fit, survival probabilities by group, monotone covariate effects (bs = "mpi"),pamm(engine = "scam"), and notes on CI computation.See attached.
shapeconstraints.html
Tests (
tests/testthat/test-scam.R, 67 assertions): hazard/SE correctness againstpredict.scam(se.fit = TRUE), monotonicity of constrained fits, all CI types for hazard/cumulative hazard/survival probability, grouped cumulative hazards,add_term()with reference, CIF with constrained by-cause smooths, cumulative coefficients,pamm(engine = "scam"),predictSurvProb, tidiers and plotting.Local verification: full test suite passes (the only 2 failures are pre-existing environment quirks in
test-warnings.Runrelated to this change, failing identically on master),R CMD checkclean apart from pre-existing NOTEs, article renders in ~9s.https://claude.ai/code/session_011bPCsxyWjbK9TzWA9M7ceH