fix(survey): unit-level survey_metadata recompute reports raw weight scale - #801
Conversation
…scale CS (panel+RC), the staggered DDD engine, ContinuousDiD (analytic branch), and EfficientDiD passed resolve()'s mean-1 normalized weights as compute_survey_metadata's raw_weights, misreporting sum_weights and weight_range (scale-invariant fields and all estimates/inference unaffected). Capture the raw weight column per family (the DMLDiD #796 pattern), snapshotting before working-frame mutations so a weight column aliasing a mutable role column (e.g. weights == dose in ContinuousDiD) still surfaces the user's original values. Metadata provenance only: estimates, SEs, p-values, CIs, df_survey, n_strata, n_psu byte-identical; effective_n/design_effect unchanged within float round-off.
Overall assessmentExecutive summary
MethodologyP1 — ContinuousDiD metadata can describe different weights from inferenceLocation: Impact: When zero-dose treated units trigger filtering and Concrete fix: Re-resolve the filtered survey design from immutable copies of its original design columns, aligned to retained rows. Alternatively, reject mutable-role aliases when re-resolution is required. Code QualityNo additional findings. PerformanceNo findings. MaintainabilityNo additional findings. Tech DebtP3 — Analytical/bootstrap metadata granularity divergenceImpact: ContinuousDiD still reports unit-level analytical metadata versus observation-level bootstrap metadata. Concrete fix: None required for approval; this is explicitly tracked in SecurityNo findings. Documentation/TestsP1 — Combined interaction is untestedLocation: Impact: Filtering and Concrete fix: Add an analytical test containing both zero-dose treated units and nonzero-dose controls. Compare Path to Approval
|
…data rows
The dose-filter re-resolve read the mutated working frame, so a design
column aliasing a mutated role column (weights == dose) zero-weighted
every never-treated unit on filtered fits ('No valid (g,t) cells') and
diverged from the unfiltered path's pristine-data resolution. Resolve on
the pristine kept-unit rows instead (row-for-row identical to df;
byte-identical on non-aliased designs) and pin the combined
filter + aliasing case against an immutable duplicate column (CI review
finding).
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
MethodologyP1 — [Newly identified] Falsy weight-column names produce incorrect metadataLocation: Impact: Concrete fix: Replace these checks with The previous ContinuousDiD P1 is resolved: filtered survey designs now resolve from pristine retained rows ( Code QualityNo additional findings. PerformanceNo findings. MaintainabilityNo additional findings. Tech DebtP3 — Tracked analytical/bootstrap metadata granularity differenceImpact: ContinuousDiD reports unit-level analytical metadata but observation-level bootstrap metadata. Concrete fix: None required for approval; tracked in SecurityNo findings. Documentation/TestsP2 — Changelog overstates byte-identical behaviorLocation: Impact: The combined ContinuousDiD alias/filter correction changes a formerly failing inference path into a valid fit, so “metadata-provenance only” is not universally accurate. Concrete fix: Qualify the byte-identity claim to previously successful, non-aliased designs and mention the alias/filter correction. Path to Approval
|
… column names) resolve() treats any non-None string as a column name, so truthiness checks on survey_design.weights silently substituted all-ones metadata for a column literally named ''. Fix the three new raw-capture sites (CI review finding) and the same pre-existing pattern at 13 sibling sites repo-wide (_resolve_survey_for_fit included); add nonuniform weights='' pins for CS RC, EfficientDiD, and ContinuousDiD. Qualify the CHANGELOG byte-identity claim to non-aliased designs and document the alias/filter correction.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
MethodologyP1 — [Newly identified] Empty-string weight handling remains incompleteLocations: Impact:
Concrete fix: Use explicit
The intended CS, staggered DDD, ContinuousDiD, and EfficientDiD metadata changes otherwise preserve the documented estimator and variance formulas. Code QualityNo additional findings. PerformanceNo findings. MaintainabilityNo additional findings. Tech DebtP3 — Tracked ContinuousDiD metadata granularity differenceImpact: Analytical metadata is unit-level while bootstrap metadata remains observation-level. Concrete fix: None required for approval; tracked in SecurityNo findings. Documentation/TestsNo separate findings. The prior changelog overstatement is corrected. Runtime tests were unavailable; all 16 changed Python files parsed successfully. Path to Approval
|
…am, WooldridgeDiD) SunAbraham's cohort-aggregation weight-column resolution and WooldridgeDiD's pre-exclusion zero-weight-group validation still used truthiness, so a column literally named '' silently fell back to unweighted cohort mass (SA: moved att AND se, execution-verified) or skipped the early zero-weight rejection (Wooldridge). Both now check 'is not None' like the rest of the surface; exhaustive grep finds no remaining weight-name truthiness (HAD's isinstance(str) guard already handled ''). Adds SA ''-vs-named bit-parity (overall + event study) and a Wooldridge zero-weight-comparison-unit parity pin (CI review round 3).
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0/P1 findings. Executive summary
MethodologyNo findings. Affected methods—CallawaySantAnna, staggered TripleDifference, ContinuousDiD, EfficientDiD, SunAbraham, and WooldridgeDiD—were checked against their Registry entries and cited sources. The changes correctly preserve original survey-weight scale for metadata while retaining normalized weights for estimation. The previous issues are fixed at Code QualityNo findings. The repository-wide truthiness pattern was consistently replaced with explicit PerformanceNo findings. MaintainabilityNo findings. Raw arrays are aligned using each estimator’s existing unit-order mechanism. Tech DebtP3 — Tracked ContinuousDiD metadata granularity difference
SecurityNo findings. Documentation/TestsNo findings. The Registry wording accurately distinguishes raw-scale metadata scalars from retaining a per-observation raw-weight vector. |
1 participant
Summary
survey_metadatarecompute in four families — CallawaySantAnna (panel + repeated-cross-section lanes), the staggered DDD engine (TripleDifference/StaggeredTripleDifference), ContinuousDiD (analytical branch), and EfficientDiD — which passedresolve()'s mean-1 normalized weights ascompute_survey_metadata'sraw_weights, misreportingsum_weightsandweight_rangeon the normalized scale instead of the user's original weight scale (TODO.md row, origin: DMLDiD survey PR feat(dml): DMLDiD survey-design and cluster support on both lanes (DML survey PR) #796 review; the EfficientDiD site was found by the repo-wide sibling audit — all other 17 call sites verified raw).datavia the shared_extract_unit_survey_weightshelper (their working frame hasfirst_treat/_never_treatedoverwritten); ContinuousDiD snapshots the column from pristinedatabefore any working-frame mutation (so a weight column aliasing the mutabledosecolumn still surfaces the original values) and reindexes to the dose-filtered unit order; EfficientDiD threads a fit-local raw array into_recompute_unit_survey_metadataas a parameter (no new instance state).df_survey,n_strata,n_psuare byte-identical pre/post fix (verified on a captured baseline of all affected fits);effective_n/design_effectare scale-invariant ratios, unchanged within floating-point round-off.aggregate('total')survey-gate REGISTRY parenthetical whose "retains no raw record" claim the fix made stale (no methodology change).Methodology references (required if estimator / math changes)
Validation
tests/test_survey_metadata_raw_scale.py(15 pins): raw-scalesum_weights/weight_range/effective_n/design_effectper family on deliberately non-unit-scale weights; CS RC lane (panel=False) and injected-cluster (cluster=+ PSU-less design) routes; bare-cluster=unreachability pins for the new asserts (CS both lanes + DDD engine); weights-None ones-fallback with aneffective_nlength guard (all four families); ContinuousDiD dose-filter alignment (fixture with zero-dose treated units so the filtered-frame re-resolve actually fires), weight-column-aliasing-dose regression, and a bootstrap-branch obs-level characterization pin; EfficientDiD shuffled-rows +control_group="last_cohort"alignment hardening.sum_weights/weight_rangemove to raw scale, everything else byte-identical (effective_n/design_effectwithin 1e-12).Security / privacy