feat(etwfe): unsupported_period_action opt-out for comparison-support period filtering (M-147) - #821
Open
igerber wants to merge 1 commit into
Open
feat(etwfe): unsupported_period_action opt-out for comparison-support period filtering (M-147)#821igerber wants to merge 1 commit into
igerber wants to merge 1 commit into
Conversation
… period filtering (M-147)
Overall Assessment: ✅ Looks goodExecutive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation / Tests
Runtime tests could not be executed in the review container because |
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
WooldridgeDiD(unsupported_period_action="drop" | "error"), the opt-out forper-period comparison-support filtering that ledger row M-125 listed as NOT
INCLUDED and
TODO.mdtracked. An unsupported period is one lacking therequired comparison support: no positive-weight eligible comparison observation
(never-treated on the
never_treated+ OLS branch; never-treated ornot-yet-treated elsewhere) is observed there, so no
ATT(g, t)at that periodis identified against an untreated outcome.
"drop"(default) is byte-identical to the shipped M-125 behavior: unsupportedperiods are removed before the solve and the reduction is warned. Measured on the
{3,5,8}all-eventually-treated builder:overall_att1.5416, cells(3,3..7),(5,5..7), 1470 of 1890 rows retained; the drop-warning text isunchanged against the pre-change source.
"error"refuses withValueErrorat the top of the filter block, before thesurvey_design=refusal and before any row is removed, naming the periods, thewould-be-dropped observation count and the cause. It is not gated on
rank_deficient_action: the message is identical acrosswarn/error/silentand no comparison-support, reference-movement, zero-cell or rank warning precedes
it (the pre-filter outcome-fit hint, which reads only the outcome column, can
legitimately fire first and is pinned as the documented exception).
predicate branch and every rank mode refuses anyway, because the cells at an
unsupported period sum to that period's time indicator; skipping can only yield a
rank-deficient or completeness-gate refusal with a vaguer message, never a fit.
structurally (no eligible row observed) or by zero survey weight (eligible
rows observed, all at weight 0). A shared helper
_comparison_support_causenames the right one for both the warning and the refusal; single-cause sentences
are byte-identical to the pre-existing warning, and only mixed frames carry
explicit per-cause period lists. The remedy is conditional on
survey_design=(the default also refuses under a survey design, and the message carries the
PSU/stratum caveat of the survey refusal).
stay true under both modes. No estimate, weight, variance or aggregation changes
under either value; no results field is added (a successful
"error"fit runscode identical to
"drop"; the omission is recorded in the ledger row).introduced_in: 4.0, terminal) plus the snapshot bumps(
tests/test_v4_matrix.py,docs/v4-design.md), the migration-guide bullet, theM-125 NOT-INCLUDED clause amended, REGISTRY notes (new note plus two in-place
qualifications),
llms-full.txt,llms-autonomous.txt(and its newdoc-deps.yamlentry), the API reference note, the ETWFE tutorial(markdown-only),
TODO.md(row deleted; survey row's exception-type wordingscoped), and a changelog fragment.
tests/test_doc_snippets.pynow executes snippets insidetmp_path, so the API docs' baresavefig('<name>.png')calls no longer writePNGs into the checkout. Four generated PNG artifacts were removed.
Methodology references (required if estimator / math changes)
comparison support on all-eventually-treated panels (the cell half of W2025
Section 5.4, Eqs. 5.13-5.15).
two-way Mundlak regression, and difference-in-differences estimators. Empirical
Economics, 69(5), 2545-2587. https://doi.org/10.1007/s00181-025-02807-z.
In-repo review:
docs/methodology/papers/wooldridge-2025-review.md(Section5.4 notes). External anchor: Stata
jwdidall-treated golden block(
tests/test_etwfe_cs_stata_parity.py::TestAllEventuallyTreatedVsStataJwdid),unchanged and passing.
path is unchanged and still implements the Section 5.4 mechanic;
"error"is anopt-in refusal that estimates nothing. The pre-existing, REGISTRY-documented
deviation stands: Stata
jwdidperforms the same reduction silently, thislibrary reports it. The absence of a "keep the rows" mode is a deliberate
design decision recorded in the REGISTRY note and ledger row with the measured
evidence.
Validation
tests/test_wooldridge.py(new classTestUnsupportedPeriodAction, 10 test functions / 21 collected cases: constructor contract incl. transactionalset_params; 4-branch x 3-rank-mode refusal matrix; zero-weight, mixed-cause andall-weight-one survey panels on OLS/logit/Poisson; no-op invariance; ordering
ahead of the survey refusal; default byte-identity; refusal replacing the
completeness gate; warning/refusal wording lockstep; the binary-outcome hint as
the one warning that may precede the refusal), two inline panels extracted into
shared builders with existing assertions unchanged, one default-construction
assertion;
tests/test_v4_matrix.py(M-147 snapshot + contract tally);tests/test_doc_snippets.py(CWD isolation)..venv, Python 3.14):full default suite 14095 passed / 224 skipped / 253 deselected (run before the
subsequent documentation and test revisions); after those revisions,
tests/test_wooldridge.pytests/test_v4_matrix.py+tests/test_tracking_files.py605 passed /24 skipped, and
tests/test_doc_snippets.py+tests/test_tracking_files.py206 passed / 7 skipped with no PNG produced;
tests/test_wooldridge.pyunderboth
DIFF_DIFF_BACKENDvalues (282 passed each); contract/ledger/docs/guidesuites (
test_base_estimator,test_naming_guard,test_v4_matrix,test_tracking_files,test_changelog_fragments,test_docs_ia,test_doc_deps_integrity,test_notebook_md_extract,test_guides) 988 passed /67 skipped;
tests/test_etwfe_cs_stata_parity.py33 passed;premerge_scan.py0 findings;
changelog_compile.py checkOK; secret scan clean; ruff and blackclean.
docs/tutorials/16_wooldridge_etwfe.ipynbexecuted viapytest --nbmake(1 passed, no outputs written back; the edit is markdown-only and its stored
drop-warning output remains valid under the unchanged default). In-memory
measurements behind the design (filter disabled: every branch/rank mode
refuses; zero-weight and mixed-cause reachability on both predicate branches)
are recorded in the REGISTRY note and the M-147 ledger notes.
mypy diff_diff(Python 3.10 target)stops before checking any project file in this frozen environment, on the
NumPy 2.5.2 stubs' Python 3.12
typestatements (also reproduced on unchanged source; the frozen lint workflow pins NumPy 2.4.5). Local type-checkevidence comes from an unchanged configuration run with a command-line
--python-version 3.12override: no issues in 108 source files. That isevidence for the changed code, not a substitute for the pinned CI lint lane.
Security / privacy
the working tree; nothing flagged).
Changelog
changelog.d/20260905-etwfe-unsupported-period-action.md(Added: M-147;Internal: doc-snippet CWD isolation).