v2.3.1.904: time-varying weights, fix_weights, namespace cleanup, expanded tests by pedrohcgs · Pull Request #257 · bcallaway11/did

pedrohcgs · 2026-04-03T00:09:30Z

What this PR does

This PR (v2.3.1.904) fixes a bug where faster_mode = TRUE and FALSE produced different ATT estimates with time-varying sampling weights, adds the new fix_weights parameter for explicit user control over weight resolution, cleans up namespace pollution that was masking dplyr::filter/dplyr::lag, and expands the test suite from ~330 to 687 tests with 0 warnings.

Bug fixes

Time-varying weights produced different ATTs in fast vs slow mode. The fast path (compute.att_gt2.R) always pulled weights from weights_vector (first period). Now both paths use a per-period weights_tensor so each 2×2 comparison uses the correct period's weight. (R/compute.att_gt2.R, R/pre_process_did2.R, R/DIDparams2.R)
aggte() crashed when the user's group column was literally named gname. dreamerr >= 1.5.0 intercepts data.table's get() inside [.data.table. Replaced dt[get(gname) == Inf, (gname) := 0] with set(). (R/compute.aggte.R)
faster_mode = TRUE dropped failed (g,t) cells; slow mode kept them as NA. With base_period = "varying", fast mode returned NULL for failed cells, filtered them out, and hard-stopped with "No valid (g,t) cells found." Now both modes keep failed cells as att = NA. (R/compute.att_gt2.R, lines 530–534)
glance.MP() returned NULL for ngroup/ntime with faster_mode = TRUE. DIDparams2 uses treated_groups_count/time_periods_count, not nG/nT. Added mode-aware branching. Also fixed glance.AGGTEobj() which was using nrow(cohort_counts) (overcounts by including never-treated group) — now uses treated_groups_count. (R/tidy.R)
Unbalanced panel + fix_weights = "base_period"/"first_period" crashed in slow mode. The influence function aggregation at line 599 used rightids captured before the fix_weights path dropped rows with missing reference-period weights. Replaced with current_ids from the post-drop disdat. (R/compute.att_gt.R, line 599)
est_method docs described wrong custom estimator signature. Docs said f(Y1, Y0, treat, covariates) but code actually calls f(y1, y0, D, covariates, i.weights, inffunc, ...) for panel and f(y, post, D, covariates, i.weights, inffunc, ...) for RC. Updated docs to match. (R/att_gt.R, lines 89–134)
get_wide_data() could crash with data.table .checkTypos when user columns match local variable names (e.g., a column named tname). Pre-extract data[[tname]] to avoid. (R/utility_functions.R)
Namespace pollution. Blanket import(stats), import(utils), import(BMisc) re-exported hundreds of symbols including stats::filter and stats::lag, masking dplyr. Replaced with selective importFrom(). Also added missing imports for stats::ecdf, stats::glm, stats::predict and registered .w, D, N, post, weights as globalVariables. R CMD check now has 0 code-related NOTEs. (R/imports.R, NAMESPACE)

New features

fix_weights parameter in att_gt() — controls how time-varying sampling weights are resolved in each 2×2 DiD comparison:
- NULL (default): earlier-period weight for balanced panel; per-observation for RC/unbalanced
- "varying": per-observation weights via RC estimators (blocked for custom est_method on panel)
- "base_period": fix at g-1 for all cells in a group (blocked for panel = FALSE)
- "first_period": fix at first period for all cells (blocked for panel = FALSE)
Implemented in both slow path (R/compute.att_gt.R) and fast path (R/compute.att_gt2.R) with matching influence function aggregation. (R/att_gt.R, R/DIDparams.R, R/DIDparams2.R, R/pre_process_did.R, R/pre_process_did2.R)
nobs() S3 methods for MP and AGGTEobj. (R/tidy.R)
tidy() additions: statistic (t-stat) and p.value (pointwise, two-sided) columns for both MP and AGGTEobj. (R/tidy.R)
Time-varying weight detection message when weights vary across time in balanced panel data. (R/pre_process_did.R, R/pre_process_did2.R)

CI / maintenance

New bump-version.yaml GitHub Action: auto-bumps dev version in DESCRIPTION on PR merge to master
Modernized test.yaml: replaced manual apt-get/cache with r-lib/actions/setup-r-dependencies@v2
Updated test-coverage.yaml: v1 → v2 actions
test-inference.R cleanup: HTTPS mirror, NOT_CRAN guard around network install, withr::defer for temp directory cleanup

Test suite

6 new test files added:

test-aggte-comprehensive.R — all aggregation types, na.rm, min_e/max_e/balance_e
test-edge-cases.R — single group, two periods, no never-treated, unbalanced
test-error-handling.R — validation errors, warnings, messages
test-faster-mode-consistency.R — 36-combination grid: est_method × panel_type × control_group × base_period
test-ggdid.R — plot generation for all aggregation types
test-glance.R — glance.MP and glance.AGGTEobj for both faster_mode settings

Existing tests modified: suppressWarnings() on expected cband/Wald warnings to eliminate noise. Result: 0 FAIL, 0 WARN, 1 SKIP (known DRDID bug), 687 PASS.

Test plan

devtools::test() — 687 PASS, 0 FAIL, 0 WARN
devtools::check() — 0 errors, 0 warnings, 3 NOTEs (all environmental: covr not installed, installed size, NTP)
Time-varying weights: fast/slow ATT parity across all est_method × base_period × control_group
All fix_weights options tested on balanced, unbalanced, and RC panels
Unbalanced panel + fix_weights with units missing from reference period: no crash, fast/slow match
Validation: custom est_method + "varying" blocked; "base_period"/"first_period" blocked for RC
Influence function aggregation verified correct in both code paths (rowsum in slow, half-split in fast)

…amespace pollution, fix dreamerr/get() crash - Fix faster_mode mismatch with time-varying sampling weights on balanced panel - Add fix_weights parameter: NULL (default), "varying", "base_period", "first_period" - Replace import(stats), import(utils), import(BMisc) with selective importFrom - Fix aggte() crash from dreamerr intercepting get() inside data.table - Add runtime message for time-varying weights detection - Skip inference tests gracefully when did v2.1.2 unavailable from CRAN - Add GitHub Action to auto-bump version on PR merge - 236 PASS, 0 FAIL, 8 SKIP

…sistency tests The half-split aggregation of the RC influence function in compute.att_gt.R incorrectly paired pre/post contributions across different units, producing wrong standard errors. Replace with rowsum() by unit ID for correct, order-independent aggregation. This also fixes a pre-existing SE discrepancy between faster_mode=TRUE and faster_mode=FALSE when fix_weights="varying" (~2x SE inflation in slow mode). Add 72 new tests verifying both ATT and SE match between slow/fast modes across all fix_weights options, est_methods, base_periods, control groups, data types (panel, RC, unbalanced), and covariate specifications. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

… HTTPS mirror - test.yaml: replace hand-rolled install with r-lib/actions/setup-r-dependencies (fixes "remotes is required" error from deprecated devtools::install_deps) - bump-version.yaml: add concurrency group to prevent race conditions on concurrent PR merges - test-inference.R: use HTTPS CRAN mirror, verify install with requireNamespace Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Upgrade from r-lib/actions v1 (deprecated) to v2 to match the rest of the CI workflows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

This reverts commit 6abc765.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…lete namespace cleanup, suppress test warnings - Block fix_weights="varying" with custom est_method (breaks calling convention) - Add missing @importFrom: stats::ecdf, stats::glm, stats::predict - Register .w, D, N, post, weights as globalVariables (R CMD check clean) - Replace fragile exists("use_rc_for_weights") with direct dp2 check - Revert GH Actions workflow to reliable pull_request:closed trigger - Fix glance.MP/AGGTEobj for faster_mode field names - Fix get_wide_data .checkTypos, pre_process_did2 by=get() -> by=c() - Suppress expected warnings in all test files (0 WARN, 1 SKIP, 610 PASS) - Add 6 new test files: aggte, edge-cases, error-handling, faster-mode, ggdid, glance

…d of dropping Previously, faster_mode=TRUE would drop (g,t) cells that failed estimation when base_period="varying", while slower mode kept them as NA. This caused faster_mode=TRUE to hard-stop with "No valid (g,t) cells found" when all cells failed (e.g., singular RC covariate designs with fix_weights="varying"), while slower mode returned an MP with all-NA ATTs.

into v2.3.1.902-bugfixes

…e docs - Fix test my_rc_est: capture length(y) before subsetting to avoid returning a short influence function (sum(D==0) vs n_obs), which caused silent recycling warnings masked by blanket suppressWarnings() - Replace suppressWarnings with withCallingHandlers that promotes recycling warnings to errors while muffling expected Wald pre-test warns - Guard fix_weights="varying" + custom est_method rejection to panel=TRUE only, so the RC path works correctly with custom estimators - Restore geom_errorbarh() in splot() for horizontal group-time plots - Update est_method docs (roxygen + Rd) to describe both panel and RC custom-estimator signatures and correct return field name (att.inf.func) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…e docs - Fix unbalanced panel crash with fix_weights="base_period"/"first_period" in slow mode: use current disdat IDs instead of stale rightids for influence function aggregation (fixes length mismatch error) - Fix est_method docs: signature was Y1,Y0,treat but code calls y1,y0,D

…e docs - Fix est_method docs: panel signature was f(Y1,Y0,treat,...) but code calls f(y1,y0,D,covariates,i.weights,inffunc,...). Updated to match. - Add test for unbalanced panel + fix_weights with units missing from reference period (exercises the row-dropping + IF aggregation path)

Copilot

Pull request overview

This PR updates the did package to improve correctness and API clarity around time-varying sampling weights, while also reducing namespace pollution and substantially expanding automated test coverage (including fast/slow mode parity).

Changes:

Fixes fast/slow mode ATT parity when weightsname varies over time and introduces fix_weights to control weight resolution behavior.
Cleans up package imports (importFrom()), fixes a get()/column-name collision crash, and improves glance() output consistency in faster_mode.
Adds multiple new test files covering edge cases, error handling, plotting helpers, aggregations, and fast/slow consistency.

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/testthat/test-user_bug_fixes.R	Wraps known-warning calls to stabilize expectations.
tests/testthat/test-jel_replication.R	Suppresses expected warnings in replication tests; adds parity checks.
tests/testthat/test-inference.R	Adds install-availability gating for old `did` version and skips when unavailable.
tests/testthat/test-glance.R	New tests for `glance.MP()`/`glance.AGGTEobj()` across `faster_mode`.
tests/testthat/test-ggdid.R	New tests for `ggdid()` outputs and error paths.
tests/testthat/test-faster-mode-consistency.R	New systematic `faster_mode` TRUE/FALSE consistency grid tests.
tests/testthat/test-error-handling.R	New tests for validation errors, warnings, and messages.
tests/testthat/test-edge-cases.R	New boundary/edge case tests for `att_gt()`/`aggte()`.
tests/testthat/test-att_gt.R	Adds `fix_weights` and “column literally named gname/tname/idname” regression coverage.
tests/testthat/test-aggte-comprehensive.R	New comprehensive aggregation behavior + IF-length tests.
R/utility_functions.R	Makes `get_wide_data()`/`check_balance()` more robust to name collisions.
R/tidy.R	Fixes `glance()` fields under `faster_mode` and aligns group/time counts.
R/pre_process_did2.R	Adds time-varying weights message; threads `fix_weights`; builds weights tensor.
R/pre_process_did.R	Threads `fix_weights`; adds time-varying weights message in slow path.
R/imports.R	Replaces blanket imports with targeted `importFrom()`; extends `globalVariables()`.
R/DIDparams2.R	Stores `weights_tensor` + `fix_weights` in DIDparams for faster mode.
R/DIDparams.R	Adds `fix_weights` to DIDparams for slower mode.
R/compute.att_gt2.R	Implements `fix_weights` behavior in faster mode, including RC fallback for `"varying"`.
R/compute.att_gt.R	Implements `fix_weights` behavior in slower mode and adjusts IF aggregation.
R/compute.aggte.R	Replaces `get()` with `set()` to avoid column-name collision crash.
R/att_gt.R	Documents and validates `fix_weights`; updates custom `est_method` signature docs.
NEWS.md	Adds release notes for v2.3.1.904 changes.
NAMESPACE	Switches to selective imports and adds needed `importFrom()` entries.
man/pre_process_did2.Rd	Documents `fix_weights` and updated custom-estimator signatures.
man/pre_process_did.Rd	Documents `fix_weights` and updated custom-estimator signatures.
man/DIDparams.Rd	Documents `fix_weights` and weight-handling behavior.
man/conditional_did_pretest.Rd	Updates `weightsname` documentation to reflect time-varying behavior.
man/att_gt.Rd	Documents `fix_weights` and updated custom-estimator signatures.
DESCRIPTION	Bumps version to 2.3.1.904.
.github/workflows/test.yaml	Modernizes R CI actions and dependency installation.
.github/workflows/test-coverage.yaml	Updates coverage workflow to v2 actions + improved covr invocation.
.github/workflows/bump-version.yaml	Adds workflow to auto-increment dev version on PR merge.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-03T00:17:07Z

 temp_lib <- tempfile()
 dir.create(temp_lib)
-remotes::install_version("did", version = "2.1.2", lib = temp_lib, repos = "http://cran.us.r-project.org")
+old_did_available <- tryCatch({
+  remotes::install_version("did", version = "2.1.2", lib = temp_lib, repos = "https://cloud.r-project.org", quiet = TRUE)
+  isTRUE(requireNamespace("did", lib.loc = temp_lib, quietly = TRUE))
+}, error = function(e) FALSE)
+
+if (!old_did_available) {
+  # Clean up and skip all tests in this file
+  unlink(temp_lib, recursive = TRUE)
+}


remotes::install_version() is executed at file load time (outside any test_that) and without a CRAN/offline guard. This will still attempt a network install during R CMD check even though the tests later skip_if(!old_did_available, ...), which can fail in restricted environments. Consider moving the install into the relevant test_that blocks and guarding it with skip_on_cran() / skip_if_offline() (or equivalent), so no network calls occur when checks disallow internet.

Copilot · 2026-04-03T00:17:08Z

+old_did_available <- tryCatch({
+  remotes::install_version("did", version = "2.1.2", lib = temp_lib, repos = "https://cloud.r-project.org", quiet = TRUE)
+  isTRUE(requireNamespace("did", lib.loc = temp_lib, quietly = TRUE))
+}, error = function(e) FALSE)
+
+if (!old_did_available) {
+  # Clean up and skip all tests in this file
+  unlink(temp_lib, recursive = TRUE)
+}


Cleanup of temp_lib is only done immediately when the install fails. When the install succeeds, the temp library directory will still contain installed packages and should be cleaned up reliably (e.g., via on.exit()/withr::defer()) using unlink(temp_lib, recursive = TRUE) to avoid leaving behind directories/artifacts in CI.

Suggested change

old_did_available <- tryCatch({

remotes::install_version("did", version = "2.1.2", lib = temp_lib, repos = "https://cloud.r-project.org", quiet = TRUE)

isTRUE(requireNamespace("did", lib.loc = temp_lib, quietly = TRUE))

}, error = function(e) FALSE)

if (!old_did_available) {

# Clean up and skip all tests in this file

unlink(temp_lib, recursive = TRUE)

}

on.exit(unlink(temp_lib, recursive = TRUE), add = TRUE)

old_did_available <- tryCatch({

remotes::install_version("did", version = "2.1.2", lib = temp_lib, repos = "https://cloud.r-project.org", quiet = TRUE)

isTRUE(requireNamespace("did", lib.loc = temp_lib, quietly = TRUE))

}, error = function(e) FALSE)

Copilot · 2026-04-03T00:17:08Z

+            post_rc <- as.numeric(disdat_long[[tname]] == tlist[t + tfac])
+            w_rc <- disdat_long$.w
+            covariates_rc <- model.matrix(xformla, data = disdat_long)
+            n1_rc <- sum(G_rc + disdat_long$.C) # careful: n1 for RC is different


n1_rc is computed but never used. This looks like leftover scaffolding from the RC-weight path and adds noise to a performance-sensitive inner loop; please remove it (or use it if it was intended for scaling/checks).

Suggested change

n1_rc <- sum(G_rc + disdat_long$.C) # careful: n1 for RC is different

Copilot · 2026-04-03T00:17:09Z


  # Determine the maximum number of time periods for any unit
-  max_time_periods <- data[, uniqueN(get(time_col))]
+  max_time_periods <- length(unique(data[[time_col]]))


check_balance() now computes max_time_periods via length(unique(data[[time_col]])). Since this function is likely used on large data.tables, consider using data.table::uniqueN(data[[time_col]]) (no get() needed) to keep this O(n) and avoid allocating the full unique vector.

Suggested change

max_time_periods <- length(unique(data[[time_col]]))

max_time_periods <- data.table::uniqueN(data[[time_col]])

…eanup - Remove unused n1_rc variable in fix_weights="varying" RC path - Use data.table::uniqueN() instead of length(unique()) in check_balance - Fix test-inference.R: withr::defer for temp_lib cleanup, NOT_CRAN guard around network install, remove stale unlink at EOF

pedrohcgs · 2026-04-03T01:50:52Z

@marcelortizv review

geom_errorbarh() was deprecated in ggplot2 4.0.0. The replacement produces identical plots with no deprecation warning.

…rorbarh - Use binomial(link="logit") instead of bare logit symbol in trimmer() - Version-gate geom_errorbar(orientation="y") to ggplot2 >= 3.3.0, falling back to geom_errorbarh() on older installations

- Move overlap/rank checks inside fix_weights="varying" branch to run on RC data (covariates_rc, G_rc) instead of wide panel data - Add droplevels() on disdat_long before model.matrix() to prevent phantom zero-column dummies from unused factor levels - Keep panel guards in non-varying path (moved inside tryCatch) - Restore fix_weights="varying" + custom est_method block for all panel data (remove allow_unbalanced_panel bypass that left balanced panels exposed to wrong-signature dispatch) - Remove misleading "set panel=FALSE" workaround from error message - Workflow: scope idempotency check to base branch only, not --all refs

…vior - Use grep -qF for exact PR number match in idempotency check to prevent PR #1 from matching PR #10, #100, etc. - Document that fix_weights="varying" also evaluates covariates at each period (not just weights), since it uses the RC estimator

The "varying" option should only change weights, not the covariate conditioning set. Both code paths now use pre-period covariates for all observations in the stacked RC data, matching the panel estimator's covariate handling. Post-treatment covariates are never used. Fast path: rbind(cov_pre, cov_pre) instead of rbind(cov_pre, cov_post) Slow path: match() lookup to map each observation to its unit's pre-period covariates regardless of row ordering in disdat_long

With base_period="universal", pret can be later than the current time period for placebo cells. The varying path was always using covariates from pret, which meant conditioning on future covariates for placebo cells. Now uses min(pret, t) to match the panel estimator's convention: always the earlier of the two comparison periods.

- Add skip_on_cran() to each test_that in test-inference.R (more idiomatic than relying on the top-level NOT_CRAN guard alone) - Remove misleading "Added broom to Suggests" NEWS entry (broom was added in 2.3.1.903, not 2.3.1.904) - Add test exercising splot() via ggdid(agg_grp) to cover the ggplot2 version-gated errorbar layer and guard against deprecation warnings

pedrohcgs and others added 13 commits April 1, 2026 13:18

Fix test-coverage workflow: update to v2 actions

6abc765

Upgrade from r-lib/actions v1 (deprecated) to v2 to match the rest of the CI workflows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Revert "Fix test-coverage workflow: update to v2 actions"

f44e401

This reverts commit 6abc765.

Fix test-coverage workflow: update to v2 actions

03eff17

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Replace deprecated geom_errorbarh() with geom_errorbar() in splot()

09fcb4c

Merge branch 'v2.3.1.902-bugfixes' of https://github.com/bcallaway11/did

60e2e32

into v2.3.1.902-bugfixes

Copilot AI review requested due to automatic review settings April 3, 2026 00:09

Copilot started reviewing on behalf of pedrohcgs April 3, 2026 00:10 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

pedrohcgs added 7 commits April 3, 2026 15:02

Replace deprecated geom_errorbarh() with geom_errorbar(orientation="y")

cc68339

geom_errorbarh() was deprecated in ggplot2 4.0.0. The replacement produces identical plots with no deprecation warning.

Fix bare logit symbol, add ggplot2 version-gated fallback for geom_er…

def1de9

…rorbarh - Use binomial(link="logit") instead of bare logit symbol in trimmer() - Version-gate geom_errorbar(orientation="y") to ggplot2 >= 3.3.0, falling back to geom_errorbarh() on older installations

pedrohcgs merged commit 556a81c into master Apr 5, 2026
6 checks passed

github-actions Bot added a commit that referenced this pull request Apr 5, 2026

Bump version to 2.3.1.905 (PR #257)

1dbba48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2.3.1.904: time-varying weights, fix_weights, namespace cleanup, expanded tests#257

v2.3.1.904: time-varying weights, fix_weights, namespace cleanup, expanded tests#257
pedrohcgs merged 21 commits into
masterfrom
v2.3.1.902-bugfixes

pedrohcgs commented Apr 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

pedrohcgs commented Apr 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	max_time_periods <- length(unique(data[[time_col]]))
	max_time_periods <- data.table::uniqueN(data[[time_col]])

Conversation

pedrohcgs commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Bug fixes

New features

CI / maintenance

Test suite

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

pedrohcgs commented Apr 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pedrohcgs commented Apr 3, 2026 •

edited

Loading