Draft FPC#1193
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements Finite Population Correction (FPC) for mixed models and refactors the Kenward-Roger degrees of freedom calculation for better readability and performance. The code review identified a logic error in the FPC factor calculation where population sizes equal to sample sizes were not handled correctly. Additionally, feedback was provided to maintain sparse matrix structures to prevent excessive memory usage in large models and to ensure that all external dependencies, specifically the 'Matrix' package, are verified before use.
| if (fpc1 > n) { | ||
| fpc1 <- 1 - n / fpc1 | ||
| } | ||
| if (fpc2 > n_grplevel) { | ||
| fpc2 <- 1 - n_grplevel / fpc2 | ||
| } |
There was a problem hiding this comment.
The logic for converting population sizes to finite population correction (FPC) factors should use >= instead of >. If the population size provided equals the sample size (
# adjust population size factor when it's larger than or equal to actual sample or group size
if (!is.null(vcov_args$popsize_level1) && fpc1 >= n) {
fpc1 <- 1 - n / fpc1
}
if (!is.null(vcov_args$popsize_level2) && fpc2 >= n_grplevel) {
fpc2 <- 1 - n_grplevel / fpc2
}| # Combine all random effect terms into the final global Cholesky matrix | ||
| L_matrix <- Matrix::bdiag(L_list) | ||
|
|
||
| as.matrix(t(L_matrix / stats::sigma(model))) |
There was a problem hiding this comment.
Similar to the previous point, converting the sparse block-diagonal Cholesky factor to a dense matrix using as.matrix() is unnecessary and potentially memory-intensive. Keeping it sparse allows the subsequent multiplication on line 234 to remain efficient and reduces the overall memory footprint of the FPC calculation.
t(L_matrix / stats::sigma(model))|
Should also add a method for vcovFPC.lm <- function(object, popsize = NULL, ..., verbose = TRUE) {
N <- insight::n_obs(object)
V <- stats::vcov(object)
if (popsize <= N) {
if (verbose) {
insight::format_alert("No FPC needed.")
}
return(V)
}
fpc <- (popsize - N) / (popsize - 1)
return(V / fpc)
} |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Pull request overview
This PR adds finite population correction (FPC) support to insight::get_varcov() (and related docs/tests), including optional Kenward–Roger adjustment, with the intent of enabling FPC-adjusted fixed-effect uncertainty estimates for (at least) two-level mixed models.
Changes:
- Add new
"fpc"option toget_varcov()and route it through.get_varcov_sandwich()to a newvcovFPC()implementation. - Introduce
vcovFPC()S3 generic + methods (mixed models, lm/glm, glmmTMB alias) and add tests validating results against a reference implementation. - Update documentation (Rd + roxygen + NEWS) for
"fpc"and minor KR wording fixes.
Reviewed changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
R/get_varcov.R |
Documents new vcov = "fpc" option and adds reference. |
R/get_varcov_sandwich.R |
Adds "fpc" shortcut mapping to vcovFPC() and tweaks error formatting. |
R/get_varcov_fpc.R |
New implementation of vcovFPC() (FPC + optional KR) and helper for transposed Cholesky extraction. |
tests/testthat/test-vcov_fpc.R |
New tests for FPC behavior and error cases. |
R/get_df.R |
Minor doc wording change (“Kenward-Roger”). |
R/get_df_kenward-rogers.R |
Refactor/commenting improvements in KR ddf internals. |
NEWS.md |
Announces new "fpc" support in get_varcov(). |
NAMESPACE |
Exports vcovFPC() and registers S3 methods. |
man/get_varcov.Rd |
Mirrors roxygen doc update for "fpc" + reference. |
man/get_predicted.Rd |
Mirrors vcov option docs update (includes "fpc"). |
man/get_predicted_ci.Rd |
Mirrors vcov option docs update (includes "fpc"). |
man/get_df.Rd |
Mirrors doc wording change (“Kenward-Roger”). |
DESCRIPTION |
Bumps package version. |
Files not reviewed (4)
- man/get_df.Rd: Language not supported
- man/get_predicted.Rd: Language not supported
- man/get_predicted_ci.Rd: Language not supported
- man/get_varcov.Rd: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request implements Finite Population Correction (FPC) for variance-covariance matrix estimation, adding support for lme4, glmmTMB, and basic linear models. The code review identified several critical issues, including a mathematical error where the FPC factor was used to divide rather than multiply the variance, and logic errors in population size boundary checks. High-priority feedback was also provided regarding performance, specifically advising against converting sparse matrices to dense formats to prevent memory exhaustion. Additionally, the reviewer noted that the glmmTMB implementation incorrectly relies on lme4-specific functions and suggested using internal package helpers for consistent matrix processing.
|
@gemini-code-assist Does the glmmTMB implementation support fixed effects only models? Does it check for 2 levels? |
|
Hi @mattansb, thanks for the questions. Regarding the
|
|
@strengejacke Then we should allow glmmTMB to also use the "regular" FPC when it only has fixed effects. |
|
Ok, and what about the comments on your lm-method? |
|
DANIEL YOU'RE TOO FAST! |
|
Maybe you're just too slow... 😎 |
Created on 2026-05-19 with reprex v2.1.1