Skip to content

Commit b91a485

Browse files
igerberclaude
andcommitted
fix: address CI review round 9 - last_cohort guard, scalar validation
- P1: reject control_group='last_cohort' (EfficientDiD) with survey_config (needs multi-cohort DGP, same as not_yet_treated) - P2: add psu_re_sd and fpc_per_stratum finiteness validation to SurveyPowerConfig.__post_init__ - Update REGISTRY.md to list last_cohort alongside other restrictions - Add regression tests for last_cohort rejection and scalar validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent c5f9b3c commit b91a485

3 files changed

Lines changed: 31 additions & 4 deletions

File tree

diff_diff/power.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,10 @@ def __post_init__(self) -> None:
137137
f"weight_variation must be 'none', 'moderate', or 'high', "
138138
f"got '{self.weight_variation}'"
139139
)
140+
if not np.isfinite(self.psu_re_sd) or self.psu_re_sd < 0:
141+
raise ValueError(f"psu_re_sd must be finite and >= 0, got {self.psu_re_sd}")
142+
if not np.isfinite(self.fpc_per_stratum):
143+
raise ValueError(f"fpc_per_stratum must be finite, got {self.fpc_per_stratum}")
140144
if self.icc is not None and not (0 < self.icc < 1):
141145
raise ValueError(f"icc must be between 0 and 1 (exclusive), got {self.icc}")
142146
if self.icc is not None and self.psu_re_sd != 2.0:
@@ -1997,11 +2001,11 @@ def simulate_power(
19972001
# cohort_periods/never_treated_frac overrides.
19982002
control_group = getattr(estimator, "control_group", "never_treated")
19992003
clean_control = getattr(estimator, "clean_control", None)
2000-
if control_group == "not_yet_treated":
2004+
if control_group in ("not_yet_treated", "last_cohort"):
20012005
raise ValueError(
2002-
f"survey_config does not support control_group='not_yet_treated' "
2006+
f"survey_config does not support control_group='{control_group}' "
20032007
f"(requires multi-cohort DGP). Use the custom data_generator "
2004-
f"path for survey power with not-yet-treated controls."
2008+
f"path for survey power with this control-group design."
20052009
)
20062010
if clean_control == "strict":
20072011
raise ValueError(

docs/methodology/REGISTRY.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2255,7 +2255,7 @@ n = 2(t_{α/2} + t_{1-κ})² σ² / MDE²
22552255
- **Note:** The simulation-based power registry (`simulate_power`, `simulate_mde`, `simulate_sample_size`) uses a single-cohort staggered DGP by default. Estimators configured with `control_group="not_yet_treated"`, `clean_control="strict"`, or `anticipation>0` will receive a `UserWarning` because the default DGP does not match their identification strategy. Users must supply `data_generator_kwargs` (e.g., `cohort_periods=[2, 4]`, `never_treated_frac=0.0`) or a custom `data_generator` to match the estimator design.
22562256
- **Note:** The `TripleDifference` registry adapter uses `generate_ddd_data`, a fixed 2×2×2 factorial DGP (group × partition × time). The `n_periods`, `treatment_period`, and `treatment_fraction` parameters are ignored — DDD always simulates 2 periods with balanced groups. `n_units` is mapped to `n_per_cell = max(2, n_units // 8)` (effective total N = `n_per_cell × 8`), so non-multiples of 8 are rounded down and values below 16 are clamped to 16. A `UserWarning` is emitted when simulation inputs differ from the effective DDD design. When rounding occurs, all result objects (`SimulationPowerResults`, `SimulationMDEResults`, `SimulationSampleSizeResults`) set `effective_n_units` to the actual sample size used; it is `None` when no rounding occurred. `simulate_sample_size()` snaps bisection candidates to multiples of 8 so that `required_n` is always a realizable DDD sample size. Passing `n_per_cell` in `data_generator_kwargs` suppresses the effective-N rounding warning but not warnings for ignored parameters (`n_periods`, `treatment_period`, `treatment_fraction`).
22572257
- **Note:** The analytical power methods (`PowerAnalysis.power/mde/sample_size` and the `compute_power/compute_mde/compute_sample_size` convenience functions) accept a `deff` parameter (survey design effect, default 1.0). This inflates variance multiplicatively: `Var(ATT) *= deff`, and inflates required sample size: `n_total *= deff`. The `deff` parameter is **not redundant** with `rho` (intra-cluster correlation): `rho` models within-unit serial correlation in panel data via the Moulton factor `1 + (T-1)*rho`, while `deff` models the survey design effect from stratified multi-stage sampling (clustering + unequal weighting). A survey panel study may need both. Values `deff > 0` are accepted; `deff < 1.0` (net variance reduction, e.g., from stratification gain) emits a warning.
2258-
- **Note:** The simulation-based power functions (`simulate_power/simulate_mde/simulate_sample_size`) accept a `survey_config` parameter (`SurveyPowerConfig` dataclass). When set, the simulation loop uses `generate_survey_did_data` instead of the default registry DGP, and automatically injects `SurveyDesign(weights="weight", strata="stratum", psu="psu", fpc="fpc")` into the estimator's `fit()` call. Supported estimators: DifferenceInDifferences, TwoWayFixedEffects, MultiPeriodDiD, CallawaySantAnna, SunAbraham, ImputationDiD, TwoStageDiD, StackedDiD, EfficientDiD. Unsupported (raises `ValueError`): TROP, SyntheticDiD, TripleDifference (generate_survey_did_data produces staggered cohort data incompatible with factor-model/DDD DGPs). `survey_config` and `data_generator` are mutually exclusive. `data_generator_kwargs` may not contain keys managed by `SurveyPowerConfig` (n_strata, psu_per_stratum, etc.) but may contain passthrough DGP params (unit_fe_sd, add_covariates, strata_sizes). Repeated cross-section survey power (`panel=False`) is only supported for `CallawaySantAnna(panel=False)` with a matching `data_generator_kwargs={"panel": False}`; both mismatch directions are rejected. `estimator_kwargs` may not contain `survey_design` when `survey_config` is set (use `SurveyPowerConfig(survey_design=...)` instead). Estimator settings that require a multi-cohort DGP (`control_group="not_yet_treated"`, `clean_control="strict"`) are rejected because the survey DGP uses a single cohort; use the custom `data_generator` path for these configurations. `simulate_sample_size` raises the bisection floor to `n_strata * psu_per_stratum * 2` to ensure viable survey structure and rejects `strata_sizes` in `data_generator_kwargs` (it depends on `n_units` which varies during bisection).
2258+
- **Note:** The simulation-based power functions (`simulate_power/simulate_mde/simulate_sample_size`) accept a `survey_config` parameter (`SurveyPowerConfig` dataclass). When set, the simulation loop uses `generate_survey_did_data` instead of the default registry DGP, and automatically injects `SurveyDesign(weights="weight", strata="stratum", psu="psu", fpc="fpc")` into the estimator's `fit()` call. Supported estimators: DifferenceInDifferences, TwoWayFixedEffects, MultiPeriodDiD, CallawaySantAnna, SunAbraham, ImputationDiD, TwoStageDiD, StackedDiD, EfficientDiD. Unsupported (raises `ValueError`): TROP, SyntheticDiD, TripleDifference (generate_survey_did_data produces staggered cohort data incompatible with factor-model/DDD DGPs). `survey_config` and `data_generator` are mutually exclusive. `data_generator_kwargs` may not contain keys managed by `SurveyPowerConfig` (n_strata, psu_per_stratum, etc.) but may contain passthrough DGP params (unit_fe_sd, add_covariates, strata_sizes). Repeated cross-section survey power (`panel=False`) is only supported for `CallawaySantAnna(panel=False)` with a matching `data_generator_kwargs={"panel": False}`; both mismatch directions are rejected. `estimator_kwargs` may not contain `survey_design` when `survey_config` is set (use `SurveyPowerConfig(survey_design=...)` instead). Estimator settings that require a multi-cohort DGP (`control_group="not_yet_treated"`, `control_group="last_cohort"`, `clean_control="strict"`) are rejected because the survey DGP uses a single cohort; use the custom `data_generator` path for these configurations. `simulate_sample_size` raises the bisection floor to `n_strata * psu_per_stratum * 2` to ensure viable survey structure and rejects `strata_sizes` in `data_generator_kwargs` (it depends on `n_units` which varies during bisection).
22592259

22602260
**Reference implementation(s):**
22612261
- R: `pwr` package (general), `DeclareDesign` (simulation-based)

tests/test_power.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2602,6 +2602,17 @@ def test_survey_rejects_not_yet_treated(self):
26022602
**_SIM_KW,
26032603
)
26042604

2605+
def test_survey_rejects_last_cohort(self):
2606+
"""control_group='last_cohort' rejected (needs multi-cohort DGP)."""
2607+
with pytest.raises(ValueError, match="last_cohort"):
2608+
simulate_power(
2609+
EfficientDiD(control_group="last_cohort"),
2610+
survey_config=_SURVEY_CFG,
2611+
n_simulations=1,
2612+
seed=42,
2613+
**_SIM_KW,
2614+
)
2615+
26052616
def test_survey_rejects_clean_control_strict(self):
26062617
"""clean_control='strict' rejected (needs multi-cohort DGP)."""
26072618
with pytest.raises(ValueError, match="clean_control.*strict"):
@@ -2629,3 +2640,15 @@ def test_survey_sample_size_rejects_strata_sizes(self):
26292640
sigma=1.0,
26302641
progress=False,
26312642
)
2643+
2644+
def test_survey_config_validation_psu_re_sd_negative(self):
2645+
with pytest.raises(ValueError, match="psu_re_sd"):
2646+
SurveyPowerConfig(psu_re_sd=-1.0)
2647+
2648+
def test_survey_config_validation_psu_re_sd_nan(self):
2649+
with pytest.raises(ValueError, match="psu_re_sd"):
2650+
SurveyPowerConfig(psu_re_sd=np.nan)
2651+
2652+
def test_survey_config_validation_fpc_nan(self):
2653+
with pytest.raises(ValueError, match="fpc_per_stratum must be finite"):
2654+
SurveyPowerConfig(fpc_per_stratum=np.inf)

0 commit comments

Comments
 (0)