fix: route TripleDifference power to panel DGP when n_periods > 2#544
Merged
Conversation
simulate_power/simulate_mde/simulate_sample_size silently ignored n_periods for DDD because the _EstimatorProfile hard-coded the cross-sectional generate_ddd_data. Route to generate_ddd_panel_data when n_periods > 2, honoring n_periods/ treatment_period and sizing the panel by n_units directly. simulate_sample_size switches from the multiple-of-8 grid to a continuous step-1 search on the panel path. Emit a UserWarning when the estimator lacks cluster="unit" (panel SEs are anti-conservative and overstate power); reject the cross-sectional-only n_per_cell key with a clear ValueError. treatment_fraction stays inert (balanced 2x2x2). Docs: REGISTRY.md PowerAnalysis notes + tutorial 06 + CHANGELOG [Unreleased]; removes the resolved TODO row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses local codex P1/P2: simulate_sample_size advertised group_frac/ partition_frac overrides for the panel DDD path but hard-coded the search floor at 16, which is infeasible for skewed splits (generate_ddd_panel_data requires every (group,partition) cell non-empty — e.g. 0.1/0.1 needs n>=55). Add _ddd_panel_viable_min_n() (mirrors the DGP's rounded stratified allocation) and use it for min_n/abs_min on the panel path; raise a clear ValueError when an n_range upper bound is below that floor. Regression tests for the unbalanced split + low n_range guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Overall Assessment Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
I did not run the test suite in this read-only review environment. |
…oor helper Addresses CI codex P2: _ddd_panel_viable_min_n() now validates group_frac/ partition_frac in (0, 1) up front (matching generate_ddd_panel_data) and raises clearly if no feasible n is found within search_max, so an out-of-range split surfaces the DGP's clear message instead of a misleading "n_range below the minimum" bracketing error. Adds a unit test for the validation. Also clarifies the tutorial-06 DDD support-table footnote (P3): simulate_sample_size starts at the registry floor of 64, but an explicit n_range can reach the 16-unit floor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good. No unmitigated P0/P1 findings. Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
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.
Summary
simulate_power/simulate_mde/simulate_sample_sizenow routeTripleDifference(DDD) power to the panel DGPgenerate_ddd_panel_datawhenn_periods > 2(previously the_EstimatorProfilehard-coded the cross-sectional 2×2×2generate_ddd_data, silently ignoringn_periods). The panel path honorsn_periods/treatment_period, sizes the panel byn_unitsdirectly, and switches the sample-size search from the multiple-of-8 grid to a continuous step-1 search.UserWarningwhen the estimator lackscluster="unit"on the panel path (within-unit serial correlation makes unclustered SEs anti-conservative → overstated power); reject the cross-sectional-onlyn_per_cellkey with a clearValueError.treatment_fractionstays inert (balanced 2×2×2);group_frac/partition_fracare overridable viadata_generator_kwargs._ddd_panel_viable_min_n, mirroring the DGP's rounded stratified allocation) sosimulate_sample_sizenever probes an infeasible (empty-cell)nunder skewed splits; raise a clear error when ann_rangeupper bound is below that floor.simulate_powerdefaults ton_periods=4, the default DDD power call now uses the panel DGP (intentional — removes the prior "n_periods ignored" wart).Resolves the deferred
TODO.mdrow (Methodology/Correctness, "TripleDifference power auto-routing").Methodology references (required if estimator / math changes)
TripleDifferenceDGP routinggenerate_ddd_panel_data(DDD-CPT triple-interaction identification). No estimator math changed — this is DGP selection/wiring for the simulation power harness.docs/methodology/REGISTRY.md§PowerAnalysis (**Note:**labels) — cross-sectional (n_periods ≤ 2) vs panel (> 2) routing + thecluster="unit"inference caveat.Validation
tests/test_power.py): re-targeted two stale-warning tests + then_per_cellcollision test to the cross-sectional path; added panel-routing, missing-cluster warning, no-warning-with-cluster,n_per_cell-reject, and slowsimulate_mde/simulate_sample_sizepanel tests (incl. unbalanced-split viable-floor + low-n_rangeguard). Fulltests/test_power.pygreen (214 passed, incl. slow).docs/tutorials/06_power_analysis.ipynbDDD section updated to teach the routing + showcase the panel path withcluster="unit"; executes end-to-end (all 32 code cells).Security / privacy
Generated with Claude Code