Check the df cache budget before factoring C (Copilot review of #126) - #127
Merged
Merged
Conversation
pffr_influence_core() computed and verified a full chol(C) unconditionally before testing whether the per-cluster residualization blocks would even fit the df_precompute_bytes budget. That defeated the documented opt-out (df_precompute_bytes = 0) and could add an unnecessary O(p^3) cost. Move the byte-budget test ahead of the factorization; the verification and fallback behavior are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSoB6DyAfeyS9PytuzQs3x
There was a problem hiding this comment.
🔵 Needs a closer look
A moderate issue remains for zero-budget, all-zero-rank designs.
Pull request overview
This PR checks the df cache budget before factoring C and adds regression coverage.
Changes:
- Gates
chol(C)on the cache budget. - Tests zero-budget behavior and numerical equivalence.
- An all-zero-rank case still calls
chol(C)at zero budget and needs a guard plus boundary test.
File summaries
| File | Summary |
|---|---|
R/pffr-influence.R |
Checks the cache budget before factoring C; all-zero-rank handling remains unresolved. |
tests/testthat/test-pffr-inference-core.R |
Adds zero-budget and equivalence tests; needs all-zero-rank coverage. |
Review details
Suppressed comments (1)
R/pffr-influence.R:188
- When every cluster has rank 0,
sum(rank)is zero, so this condition is true even fordf_precompute_bytes = 0and line 189 still callschol(C). No cached blocks are needed in that case, so the zero-budget opt-out still pays the O(p^3) factorization for an all-zero design. Add a nonzero-rank guard (and cover this boundary case in the test).
within_budget <- 8 * p * sum(rank) <= df_precompute_bytes
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Addresses the Copilot review on #126:
pffr_influence_core()computed and verifiedchol(C)before testing the cache budget, sodf_precompute_bytes = 0still paid the O(p^3) factorization. The budget test now runs first; verification and the fallback path are unchanged. A test mocksbase::cholto assert it is never called at zero budget and that df / expected sampling variance match the cached path to 1e-12.🤖 Generated with Claude Code
https://claude.ai/code/session_01QSoB6DyAfeyS9PytuzQs3x