Skip to content

fix: review findings for R/r3d_main.R#15

Open
Davidvandijcke wants to merge 3 commits intomainfrom
fix/review-R-r3d_main
Open

fix: review findings for R/r3d_main.R#15
Davidvandijcke wants to merge 3 commits intomainfrom
fix/review-R-r3d_main

Conversation

@Davidvandijcke
Copy link
Copy Markdown
Owner

Automated fixes from code review pipeline.

Summary

  • F1: Use T-regression weights (wTplus/wTminus) for e2_mat row indices instead of outcome weights — prevents wrong observation selection when h_den differs from h_num
  • F2: Add positivity guards (> 0) for all user-supplied bandwidth paths — prevents divide-by-zero in Fortran
  • F3: Remove duplicate out$w_plus/out$w_minus assignments (dead code from copy-paste)
  • F4: Remove duplicate out$X/out$Y_list/out$T top-level fields (already in $inputs); update summary.r3d and print.r3d to read from $inputs$X
  • F5: Replace length-branching test validation with match.arg(..., several.ok=TRUE) for uniform partial matching at any vector length
  • F8: Add IF (INFO .NE. 0) RETURN after each DGETRS call in locweights.f so mid-loop LAPACK failures surface rather than being silently overwritten

Test plan

  • devtools::check() passes with 0 errors, 0 warnings (4 pre-existing NOTEs)
  • New test: F1 — fuzzy RD with h_den < h_num verifies e2_mat rows outside T-bandwidth are zero
  • New test: F2 — bandwidths=list(num=0) / bandwidths=-0.5 / den=0 all produce clear error
  • New test: F4 — top-level $X/$Y_list/$T absent; accessible via $inputs
  • New test: F5 — test=c("null","homo") partial matching works same as full names

🤖 Generated with Claude Code

Davidvandijcke and others added 3 commits March 5, 2026 00:57
…scope, saving CI)

S1:  Fix double-dereference ``fuzzy'' / ``weights'' -> `fuzzy' / `weights'
S7:  Read r(pilot_den) and r(h_den) into locals immediately after Mata call
     before intervening commands can clear r() results
S8:  Track Mata loading success via local mata_loaded instead of ambient _rc
S18: Remove redundant tempvar drops in r3d_bwselect.ado
S21: saving() now writes bootstrap CB bounds instead of pointwise normal CI

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- F1: extract wTplus/wTminus from T-regression Fortran output; use
  T-bandwidth weights (not outcome weights) when indexing e2_mat rows
- F2: add positivity guards for user-supplied bandwidths in all three
  validation branches (fuzzy list, sharp list, direct scalar)
- F3: remove duplicate out\$w_plus / out\$w_minus assignments (dead code)
- F4: remove duplicate top-level out\$X, out\$Y_list, out\$T to avoid
  double memory use; update summary.r3d and print.r3d to read from
  object\$inputs\$X instead
- F5: replace length-branching test validation with a single
  match.arg(..., several.ok=TRUE) call for uniform partial matching
- F8: add IF (INFO .NE. 0) RETURN after each DGETRS call in
  locweights.f so mid-loop LAPACK failures surface correctly
- Tests: add four regression tests covering F1/F2/F4/F5 findings

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

1 participant