v2.3.1.902: bug fixes, diagnostics, and JEL replication tests

[pedrohcgs]

Summary

• Fix xgap bug in ggdid plots (xgap Issue #204, Possible bug on "xgap" argument while using "ncols" in the ggdid() function. #212): xgap parameter was indexing into raw rows instead of unique year values
• Fix honest_did partial matching (event time argument in honest_did function #119): renamed parameter e to e_time to avoid R partial argument matching with es
• Fix hardcoded significance (Confidence intervals in plots #160): ggdid.AGGTEobj was hardcoding 5% significance instead of using user's alp parameter
• Add missing overlap/regression checks (compute.att_gt trimming due to pscore_problems_likely not applied when panel=FALSE #193): propensity score and regression checks were absent from panel=FALSE and faster_mode paths
• Improve diagnostics: "No pre-treatment periods" upgraded to warning() with guidance; "Dropped N units" now mentions anticipation when relevant
• Fix flaky CI test: set seeds inside callr subprocesses for deterministic bootstrap SE comparison
• Add JEL replication tests: exact point estimate verification against published Callaway & Sant'Anna JEL article

Test plan

• devtools::test() passes (0 FAIL | 246 PASS + 7 skips)
• JEL replication tests pass (41 tests, all point estimates match to 1e-6)
• R CMD check passes (0 errors, 0 warnings)
• faster_mode matches regular mode on JEL data

[pedrohcgs]

@copilot review this PR

[Copilot]

Pull request overview

This PR bumps the did package version and delivers a set of bug fixes and diagnostics improvements across plotting, inference reproducibility, overlap/regression checks, and adds optional replication tests against published JEL results.

Changes:

• Fix plotting and inference behavior: xgap handling in ggdid/gplot, significance level usage in ggdid.AGGTEobj, and deterministic bootstrap SE comparisons in inference tests.
• Improve robustness/diagnostics: add overlap/regression feasibility checks on previously uncovered estimation paths; upgrade “no pre-treatment periods” from message() to actionable warning().
• Add (skippable) JEL replication tests to validate point estimates against the Callaway & Sant’Anna JEL article.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/testthat/test-jel_replication.R	Adds skippable replication tests validating results vs published JEL point estimates.
tests/testthat/test-inference.R	Sets seeds inside callr subprocesses (and before aggregations) to reduce CI flakiness.
R/pre_process_did2.R	Improves “dropped first-period treated units” warning by noting anticipation when relevant.
R/pre_process_did.R	Same diagnostic improvement as above for the v1 preprocessing path.
R/honest_did/honest_did.R	Renames event-time argument to avoid partial matching collisions (e_time).
R/gplot.R	Adjusts xgap behavior to use unique year values rather than raw rows.
R/ggdid.R	Uses user-provided alp (instead of hardcoded 5%) when computing plot critical values.
R/compute.att_gt2.R	Adds missing overlap/regression checks in additional estimation paths.
R/compute.att_gt.R	Adds missing overlap/regression checks for panel=FALSE / unbalanced paths.
R/att_gt.R	Upgrades “no pre-treatment periods” diagnostic to a warning with more guidance.
DESCRIPTION	Version bump to 2.3.1.902.

---

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

[Copilot]

ssresults$year can be a factor (e.g., ggdid.MP sets it as as.factor(results$year)), and as.numeric(factor) returns the underlying integer codes rather than the year values. With the new unique_years <- sort(unique(as.numeric(ssresults$year))), the x-axis breaks/labels can become incorrect (labels will show codes like 1,2,3 instead of actual years). Consider converting factors via as.numeric(as.character(ssresults$year)) (or avoid converting results$year to factor upstream) so breaks reflect the actual year values.

[Copilot]

as.integer(xgap) will truncate non-integer values; for 0 < xgap < 1 it becomes 0, which makes seq(..., by=0) error. The previous implementation tolerated non-integer xgap values. Consider validating xgap is a single integer >= 1 (and error/warn otherwise), or use a safer coercion (e.g., max(1L, as.integer(round(xgap)))).

Suggested change

	dabreaks <- unique_years[seq(1, length(unique_years), as.integer(xgap))]
	xgap_int <- max(1L, as.integer(round(xgap)))
	dabreaks <- unique_years[seq(1, length(unique_years), by = xgap_int)]

[Copilot]

custom_est_method <- class(est_method) == "function" can yield a logical vector when a function has additional classes (e.g., class(f) <- c("my_est", "function")), which will make if (!custom_est_method) error (the condition has length > 1). Use is.function(est_method) or inherits(est_method, "function") to ensure a single TRUE/FALSE value.

Suggested change

	custom_est_method <- class(est_method) == "function"
	custom_est_method <- is.function(est_method)

[Copilot]

The new overlap warning in this path is very generic ("overlap condition violated for this (g,t) cell") and doesn’t include the relevant group/time identifiers. In compute.att_gt() the corresponding warning includes the treated group and time period, which is much more actionable for users debugging dropped cells. Consider including cohort/group and time (or passing them into run_DRDID) so the warning identifies which (g,t) failed.

Suggested change

	warning(paste0("overlap condition violated for this (g,t) cell"))
	# try to provide more informative (g,t) information if available
	group_info <- ""
	time_info <- ""
	# attempt to infer group / cohort identifier from common column names
	treated_idx <- which(D_vec == 1)
	if (length(treated_idx) > 0) {
	# candidate columns for group / cohort
	group_cols <- c("G", "g", "group")
	group_col <- group_cols[group_cols %in% names(cohort_data)][1]
	if (!is.na(group_col)) {
	g_vals <- unique(cohort_data[treated_idx, get(group_col)])
	if (length(g_vals) == 1) {
	group_info <- paste0(", group = ", g_vals)
	}
	}
	# candidate columns for time / period
	time_cols <- c("period", "t", "time")
	time_col <- time_cols[time_cols %in% names(cohort_data)][1]
	if (!is.na(time_col)) {
	t_vals <- unique(cohort_data[treated_idx, get(time_col)])
	if (length(t_vals) == 1) {
	time_info <- paste0(", time = ", t_vals)
	}
	}
	}
	warning(paste0("overlap condition violated for this (g,t) cell",
	group_info, time_info))

[Copilot]

Same as above: this overlap warning is too generic for users to locate the problematic cell. Including the relevant cohort/group and time period (consistent with compute.att_gt()) would make diagnostics much more useful.

Suggested change

	warning(paste0("overlap condition violated for this (g,t) cell"))
	warning(sprintf("overlap condition violated for ATT(g = %s, t = %s)", g, t))

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

tests/testthat/test-inference.R:671

• Only the aggregation callr subprocesses are seeded here. The earlier callr::r() subprocesses that run att_gt() (which uses bootstrap by default) still won’t inherit the parent seed, so their bootstrap-based SEs can remain non-deterministic and may continue to cause rare CI flakes.

Consider setting set.seed() inside all callr subprocess functions that rely on bootstrap, or explicitly pass bstrap = FALSE in subprocesses where bootstrap output isn’t needed.

  # aggregations — set seed inside each subprocess for reproducible bootstrap SEs
  dyn_2.1.2 <- callr::r(
    function(data, temp_lib) {
      set.seed(1234)
      library(did, lib.loc = temp_lib)
      res <- att_gt(
        yname = "Y", xformla = ~X, data = data, tname = "period", idname = "id",
        gname = "G", est_method = "ipw", panel = TRUE, allow_unbalanced_panel = TRUE

---

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

[Copilot]

This test file attempts to download the JEL dataset at file scope when it’s not present. Network access in package tests is fragile and typically disallowed on CRAN/CI environments; it can also create non-deterministic failures if the URL changes or rate-limits.

Consider removing the auto-download entirely and only running these tests when JEL_DID_DATA_PATH points to a local file (or gating download behind an explicit opt-in env var and skip_on_cran()).

[Copilot]

Renaming e to e_time doesn’t fully resolve the partial-argument-matching issue described in #119 when users call honest_did(obj, e = 0): the name e can still partially match the first formal argument es (and now may be ambiguous between es and e_time).

To actually prevent this, rename the first formal argument in the method away from es (e.g., object) so e can only match e_time, and optionally add a deprecation shim for the old e name if you want backward compatibility.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

---

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

[Copilot]

This test file performs download.file() at top-level during sourcing, which will run even when individual tests are later skipped via skip_on_cran(). This can violate CRAN/offline testing policies and can make checks flaky/slow in restricted-network environments. Consider gating any download behind an explicit opt-in env var (e.g., require JEL_DID_DATA_PATH to be set) and/or moving the download logic inside test_that() after skip_on_cran() so that no network access is attempted by default.

Suggested change

	# Path to the JEL-DiD data file: check env var, local path, or download from GitHub
	jel_data_path <- Sys.getenv("JEL_DID_DATA_PATH",
	unset = file.path(path.expand("~"), "JEL-DiD", "data", "county_mortality_data.csv"))
	if (!file.exists(jel_data_path)) {
	jel_data_path <- file.path(tempdir(), "county_mortality_data.csv")
	if (!file.exists(jel_data_path)) {
	tryCatch({
	download.file(
	"https://raw.githubusercontent.com/pedrohcgs/JEL-DiD/main/data/county_mortality_data.csv",
	destfile = jel_data_path,
	quiet = TRUE
	)
	}, error = function(e) {
	# If download fails, tests will be skipped via skip_if_not below
	})
	}
	}
	jel_data_available <- file.exists(jel_data_path)
	# Helper to load and clean JEL data
	load_jel_data <- function(path, filter_2xt = TRUE) {
	# Path to the JEL-DiD data file: check env var or local path (no automatic download)
	jel_data_path <- Sys.getenv(
	"JEL_DID_DATA_PATH",
	unset = file.path(path.expand("~"), "JEL-DiD", "data", "county_mortality_data.csv")
	)
	jel_data_available <- file.exists(jel_data_path)
	# Helper to load and clean JEL data
	load_jel_data <- function(path, filter_2xt = TRUE) {
	load_jel_data <- function(path, filter_2xt = TRUE) {

[Copilot]

In the panel=FALSE path, these early returns build inf_func as rep(NA_real_, n) where n is nrow(cohort_data) (number of observations). When dp2$allow_unbalanced_panel is TRUE, downstream code expects the influence-function vector length to be dp2$id_count (unique units, after aggregating by .rowid), so returning length n can cause dimension mismatches or errors when assembling the sparse influence-function matrix. Adjust these early returns to return an inf_func of the same length as the normal path (typically dp2$id_count when allow_unbalanced_panel, otherwise n).

[Copilot]

honest_did() / honest_did.AGGTEobj() are marked with @export, but the committed NAMESPACE currently does not include export(honest_did) or an S3method(honest_did, AGGTEobj) entry (and there is no corresponding .Rd file under man/). If this PR intends these to be public, please regenerate and commit the roxygen outputs (NAMESPACE + docs) so the exports/method registration actually take effect in the built package.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

---

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

[Copilot]

gplot() now coerces ssresults$year via as.numeric(as.character(...)), which will produce NA for non-numeric time period labels (e.g., "2009Q1", "T1") and break plotting/breaks. Previously, factor-to-numeric produced stable integer positions and still displayed the original labels. Consider mapping year to sequential integer positions for the x aesthetic (preserving ordering), while keeping labels from the original year values for scale_x_* breaks/labels so non-numeric time indices continue to work.

[Copilot]

get_jel_data_path() assumes a failed download.file() will throw an error, but in R it often returns a non-zero status with a warning and may still create an empty/HTML destfile. That can make file.exists(path) true and later read.csv() fail unexpectedly. Consider checking the return status (and/or file.info(path)$size > 0) and deleting the destfile on failure, and/or adding skip_if_offline()/an env-var gate so tests never attempt network access unless explicitly enabled.

Suggested change

	if (file.exists(path)) return(path)
	tryCatch({
	download.file(
	"https://raw.githubusercontent.com/pedrohcgs/JEL-DiD/main/data/county_mortality_data.csv",
	destfile = path,
	quiet = TRUE
	)
	}, error = function(e) {
	# download failed; return path anyway, skip_if_not will handle it
	})
	# If a temp file already exists, only reuse it if it is non-empty
	if (file.exists(path)) {
	info <- file.info(path)
	if (!is.na(info$size) && info$size > 0) {
	return(path)
	}
	}
	download_ok <- FALSE
	tryCatch({
	res <- download.file(
	"https://raw.githubusercontent.com/pedrohcgs/JEL-DiD/main/data/county_mortality_data.csv",
	destfile = path,
	quiet = TRUE
	)
	download_ok <<- identical(res, 0L)
	}, error = function(e) {
	# download failed; mark as unsuccessful; skip_if_not will handle missing file
	download_ok <<- FALSE
	})
	# If the download did not succeed or produced an empty file, delete it so that
	# downstream checks using file.exists(path) can correctly skip tests.
	info <- if (file.exists(path)) file.info(path) else NULL
	if (!isTRUE(download_ok) ||
	is.null(info) ||
	is.na(info$size) ||
	info$size <= 0) {
	if (file.exists(path)) {
	unlink(path)
	}
	}

[Copilot]

For readability/consistency with the rest of the codebase (which typically uses qnorm(1 - alp/2)), consider computing the pointwise critical value in that form rather than abs(qnorm(alp/2)). The current expression is mathematically equivalent but less consistent with other CI calculations (e.g., R/AGGTEobj.R).

Suggested change

	results$c <- ifelse(is.null(object$crit.val.egt), abs(qnorm(alp/2)), object$crit.val.egt)
	results$c <- ifelse(is.null(object$crit.val.egt), qnorm(1 - alp/2), object$crit.val.egt)

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

---

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

[Copilot]

honest_did() is now marked @keywords internal and no longer has @export. If this function is intended to be user-facing, it will not be available as did::honest_did() once NAMESPACE is regenerated. Consider restoring @export (and user docs) while keeping the argument rename to avoid partial matching.

[Copilot]

Similarly, honest_did.AGGTEobj() no longer has @export and is marked internal. If callers are expected to use honest_did(es, ...) externally, keep the method exported/registered so S3 dispatch works for users.