Skip to content

Speed up Satterthwaite df with empirical (sandwich) vcov - #584

Merged
danielinteractive merged 3 commits into
openpharma:mainfrom
tomonorioura:sparse-dffun-experiment
Oct 14, 2026
Merged

danielinteractive merged 3 commits into
openpharma:mainfrom
tomonorioura:sparse-dffun-experiment

Conversation

@tomonorioura

Copy link
Copy Markdown
Contributor

Closes #583.

contrast_matrix is built with Matrix::.bdiag() as a sparse block-diagonal
matrix, but was densified with as.matrix() before every use in
h_df_1d_sat_empirical(), growing quadratically with the number of subjects.
This only affects method = "Satterthwaite" with a sandwich vcov
("Empirical", "Empirical-Jackknife", "Empirical-Bias-Reduced"), which is
required for g-computation/IMMRM workflows (see #567). For models with many
subjects and coefficients, this dense allocation and multiply dominated the
per-contrast df calculation (25.4s -> 1.0s on a 240-subject, 201-coefficient
model, see #583 for details).

  • h_df_1d_sat_empirical(): accept sparse Matrix input, use
    Matrix::tcrossprod()/crossprod() to keep contrast_matrix sparse through
    the multiplication; only the final small result is densified
  • h_df_1d_sat(), h_df_md_sat(): drop the as.matrix() conversion before
    calling the helper, now that it accepts sparse input
  • the deprecated empirical_df_mat path (fits from mmrm < 0.3.15) is
    unaffected; as.matrix() is kept there since h_quad_form_mat() still
    expects a dense matrix

Verified df/SE output is unchanged on fev_data with 2-way/3-way interaction
models combined with emmeans_gcomp_vars. Full test suite passes (2862/2862).

Keep the per-subject contrast_matrix sparse through
h_df_1d_sat_empirical() instead of densifying it with as.matrix()
before every tcrossprod()/crossprod() call. Only affects
method = "Satterthwaite" with a sandwich vcov.
Bump DESCRIPTION to the development version so it matches the
0.3.19.9000 NEWS heading, following the same pattern as openpharma#578.

Reword the NEWS entry to use only vocabulary already present in the
package: "G-computation" as in the 0.3.18 entry for emmeans_gcomp_vars,
and plain wording instead of "densifying". This keeps inst/WORDLIST
unchanged.

Co-Authored-By: Lilly Code <lillycode@lilly.com>
@tomonorioura

Copy link
Copy Markdown
Contributor Author

Fixed the two failing checks that came from this branch:

The version bump also cleared the R CMD Check failure, which was the pre-existing CRAN incoming feasibility warning (Insufficient package version (submitted: 0.3.19, existing: 0.3.19)) that also fails on main; it is now a NOTE and the check passes. All checks are green.

R/satterthwaite.R is untouched by this commit.

@danielinteractive danielinteractive left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tomonorioura for the PR, please address below comments

Comment thread R/satterthwaite.R
Comment thread R/satterthwaite.R Outdated
Comment thread R/satterthwaite.R Outdated
- Reassert numeric mode on contrast_matrix for both base matrix (via
  assert_numeric) and sparse Matrix (via dMatrix class check), restoring
  the guarantee lost when assert_matrix(mode = "numeric") was replaced
  by assert_multi_class().
- Split the as.matrix(Matrix::crossprod(...)) line into two for
  readability.
- Update @PARAM contrast_matrix roxygen to reflect that a sparse
  Matrix is also accepted.
@tomonorioura

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @danielinteractive. Addressed all three in 6bcb8d9:

  • Numeric mode check: restored the guarantee lost when assert_matrix(mode = "numeric") was replaced by assert_multi_class(contrast_matrix, c("matrix", "Matrix")). Now asserts numeric mode for a base matrix via assert_numeric(), and for a sparse Matrix via a dMatrix class check (which excludes logical/pattern Matrix subclasses).
  • Readability: split as.matrix(Matrix::crossprod(...)) into two lines.
  • Documentation: updated @param contrast_matrix to note that a sparse Matrix is also accepted.

Verified h_df_1d_sat_empirical() returns an identical df value for sparse vs. densified contrast matrix input (bit-identical, confirmed with the same fit used during patch development), and all 66 tests in test-satterthwaite.R still pass.

@danielinteractive
danielinteractive merged commit 2dec33d into openpharma:main Oct 14, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Speed up Satterthwaite df with empirical (sandwich) vcov

2 participants