Repository navigation
Conversation
…ve least squares Frank-Wolfe with a budget of inner_max_iter steps stopped short of the simplex least-squares optimum on ordinary factor-model panels, so the treated fit failed closed, placebos were dropped as failed, and the nested V search ranked V by where truncated solves stopped. The problem reduces exactly to one non-negative least squares, solved here by Lawson-Hanson's active set, which terminates finitely and, being diff-diff's own, returns the same vertex at a tie under every SciPy release. The CWZ conformal proxy and rank_control_units' synthetic_weight column solve the same problem and use the same solve. Fixes #838
…-execute Tutorial 25 on top of #841 Two cv-infeasibility warnings (in-space placebo and leave-one-out) still named inner_max_iter as the budget not to raise; with the exact inner solve the only budget left is n_starts. The donor-order canonicalization comment and its test described the tie-break as Frank-Wolfe's; the active set also breaks an exact gradient tie by position (the first tied column enters), so the canonicalization is still needed and the comments now say so. Tutorial 25 is re-executed on top of #841 (treated unit in every placebo pool): the constant-effect confidence set prints [3.47, 20.45] and the nested V search keeps its uniform start exactly. The prose quotes neither number; the drift bands hold.
…s and flag a step-guard exhaustion instead of raising inner_max_iter and inner_min_decrease have had no effect since the donor weights moved to an exact non-negative least squares. Keeping them as silent no-ops hides that from a caller who tunes them; the repo precedent for an inert constructor parameter (SyntheticDiD lambda_reg, ledger M-001) is a warn-and-discard shim. Both now default to None, emit a DeprecationWarning when passed, and are stored as None (get_params and set_params see None, so a probe re-init warns once). Ledger rows M-148 and M-149 (deprecated 4.0, removed 5.0; 4.0 is the locked ladder's next release), migration-guide rows and prose, the design spec's phase-5 cell, and the naming guard's token predicate are updated together. The fit snapshot, _inner_solve_W, _v_starts, _outer_solve_V, _outer_solve_V_cv, _cwz_proxy_fit and the conformal grid helpers drop the dead budget plumbing (including the proxy's unused warm start), and the convenience function's docstring no longer lists the two names. _nnls_lawson_hanson no longer raises RuntimeError when it exhausts its step guard (50 steps per donor, not reached in practice). It returns the current feasible iterate flagged converged=False, _sc_weight_nnls threads the flag through (return_convergence=True), and _inner_solve_W and the conformal proxy report it, so the fail-closed paths that were kept (non-converged treated fit, dropped placebo, indeterminate grid point) handle it instead of a bare exception escaping from the middle of a nested V search or a confidence-interval grid. The post-fit inner-solve warning names the step guard instead of an iteration count. Tests: the budget-exhaustion test asserts a feasible flagged iterate rather than a raise; a new test monkeypatches the solve to exhaust and checks the flag reaches the fit warning, the fail-closed placebo and the conformal proxy; a new test pins the deprecation (warning text, discard to None, set_params, identical fit to the default, no warning on a default construction). Every inner_min_decrease=1e-3 in the suites, the tutorial-25 kwarg lock and the notebook's fit call drop the inert argument (the notebook's outputs are unchanged by it). Docs: REGISTRY inner-solve line and the three placebo failure notes, the llms-full signature, the changelog fragment (Deprecated section; the step-guard contract), and the resolved TODO row.
…ed on a non-finite difference, and apply the ridge per passive set _sc_weight_nnls conditioned the problem by sqrt(mean(Z**2)) on the raw donor-minus-treated block. For finite inputs beyond about 1e154 (below about 1e-154) that overflowed to inf (underflowed to 0), the division erased the fitting block, and the active set returned an arbitrary vertex flagged converged: Y = [[0, 1e200, 9e199]] gave [1, 0] instead of [0.1, 0.9], and an exact-match panel through SyntheticControl(standardize="none") put weight 1 on the wrong donor with _fit_converged still true. The scale is now computed from the max-normalized block (an RMS of O(1) numbers), so any finite magnitude gives the same weights; a non-finite Z (a difference that itself overflows) fails closed as uniform weights flagged converged=False, and a non-finite result does the same. zeta > 0 stacked a dense J x J identity under Z and carried it through every passive-set least squares (800 MB at 10,000 controls in rank_control_units). _nnls_lawson_hanson takes the ridge as a scalar and applies it inside each passive-set solve (an identity sized to the passive set) and in the gradient; the result equals the stacked form to 1e-12. The treated-fit non-convergence messages (in-space / in-time placebo, leave-one-out, the diagnostic-report reason text and the in_space_placebo docstring) attributed every non-converged fit to the outer V search and prescribed its knobs; they now name the inner step guard as the other cause and scope the remedy to the outer search. The conformal grid docstring no longer describes a warm-started Frank-Wolfe refit, and the REGISTRY donor-order Note names the active set's positional tie-break. Tests: scale invariance at 1e-300 .. 1e300 (same weights, KKT, converged); fail-closed on an overflowing difference; ridge equals the stacked identity; an exact-match fit at a 1e150 outcome scale keeps its weight, zero ATT and converged flag.
…rmal equations When the ridge dwarfs the fitting block (rank_control_units with a large lambda_reg, or a tiny outcome scale), the NNLS coefficients are O(1/ridge^2) and an SVD of the stacked [M; ridge I] system loses them: the information sits in O(1/ridge) entries of U computed to O(eps) absolute. Y = [[0, 2e-14, 1.8e-14]] with zeta = 1 returned [0.4986, 0.5014] as converged where the optimum is [0.5, 0.5] to 1e-28. With the ridge at or above the block's Frobenius norm the Gram matrix is well conditioned (cond <= 2), so the passive-set solve now uses (M'M + ridge^2 I) z = M'd there and keeps the stacked least squares otherwise; at the branch point the two agree to 1e-12. SyntheticControl and the conformal proxy run with zeta = 0 and are unaffected. The comment above test_outer_v_nonconvergence_warning no longer describes a loose inner tolerance that no longer exists.
…o heterogeneous donor scales are solved exactly A single global conditioning scale hid small donors behind a large one: with A = [[1e16, 1, 2]] and b = 1.5 the two O(1) donors fell below the active set's tolerance after the block was normalized by the 1e16 column, and the solve returned w = (0, 1, 0) (residual -0.5) as converged. Each donor column of Z is now scaled to unit norm first (B = Z D, w = D v, the sum constraint becoming (D1)'v = 1, that row itself scaled to a unit maximum), which keeps the reduction exact for any scale while giving every column its own rounding level; the active set's entering test is per column. The entering column is still chosen on the gradient in the original units (pivot_scale), so the vertex an exact-fit tie returns is the one the un-scaled reduction returned: Tutorial 25 and its drift bands are unchanged. The ridge for zeta > 0 becomes a per-column vector. A donor identical to the target gets weight 1 directly (zeta = 0). Tests: the heterogeneous-scale problem is solved to a zero residual at scales 1 and 1e+-200, as is the CWZ proxy on the same donors; an end-to-end standardize="none" fit with a 1e12 donor beside two O(1) donors reproduces the treated pre-path exactly. The tie-vertex, scale-invariance, ridge and KKT tests pass unchanged.
… and scope its magnitude claim to the float64 range Subnormal data (entries near 1e-320) makes a reciprocal column norm overflow, and entries near 1e308 make a column norm itself overflow; a NaN or inf then reaches the active set, whose comparisons are all False, so it returned the zero iterate as converged and the wrapper substituted uniform weights under that flag. Every intermediate (column norms, their reciprocals, the equilibrated block, the scale, the ridge vector and the constraint row) is now checked finite before the active set runs, and the active set itself refuses a non-finite problem; both paths return uniform weights flagged converged=False so the estimator's fail-closed paths (non-converged fit warning, skipped diagnostics) fire. The docstring and REGISTRY no longer claim finite inputs of any magnitude: the guarantee is the normal float64 range (column norms and reciprocals between about 1e-300 and 1e300), with the regimes outside it named as fail-closed. Tests: a subnormal panel and an overflowing column norm both fail closed; a NaN problem fails closed inside the active set; an end-to-end fit on subnormal outcomes reports a non-converged fit with the warning.
…rs beside a small one Two nearly opposing 1e8 donors next to an O(1) donor, with a 1e-8 wrinkle in a second predictor: the candidate optima differ in objective by about 1e-17, below the rounding of the 1e8 entries (ulp 1.5e-8), so a float64 solver can only choose among them by rounding. The test pins the contract that matters: the returned simplex vector is flagged converged, its objective is no worse than the alternative vertex evaluated in the same arithmetic, and its residual is within one ulp of the data.
…d exact solve rank_control_units() solves its simplex least squares exactly and then zeroes weights below 1e-6 and renormalizes for readability, so the column is the optimum up to that cleanup, not to the last digit (a donor whose optimal weight is below the threshold is dropped). The docstring, the solver comment and the test's name and docstring now say so; the test's assertion is unchanged (no optimal weight on its panel falls below the threshold).
Overall assessmentExecutive summary
Methodology
Code Quality
PerformanceNo findings. The passive-set ridge implementation avoids an unconditional dense donor-by-donor ridge block. MaintainabilityNo findings. Removed inner-budget plumbing is consistently propagated through nested/CV searches, placebo refits, sparse diagnostics, conformal inference, and fit snapshots. Tech Debt
SecurityNo findings. No secrets, unsafe execution paths, or sensitive-data additions observed. Documentation/Tests
Path to Approval
|
…presenting its fallback as the solution rank_control_units() called the simplex solve without its convergence flag, so a fail-closed return (a non-finite intermediate or an exhausted step guard, neither reached in practice) came back as uniform weights that were then thresholded and reported as the informational synthetic_weight column. The call now requests the flag; a non-converged solve leaves the column NaN and emits a UserWarning saying so, while quality_score and the ranking (which never used the column) are unaffected. Regression test monkeypatches the solve to fail closed. Tutorial 25 said synthetic control identifies the counterfactual path even where the weights are not identified. At an exact pre-period fit it does not: donors on the same optimal face can differ post-period, so the solver's tie-break can move the synthetic path and the ATT. The caveat and the closing checklist now say that identification rests on the factor-model assumptions and a long pre-period, and point at the robustness diagnostics for tie-break sensitivity. Markdown only; the notebook's outputs are unchanged.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
Methodology
The affected methods—ADH Synthetic Control weighting and the CWZ all-period constrained-LS proxy—continue to optimize their paper-specified simplex objectives. Analytical inference remains jointly NaN through Code Quality
PerformanceNo findings. MaintainabilityNo findings. Deprecated inner-solver parameters are consistently removed from snapshots and downstream solver paths. Tech Debt
SecurityNo findings. Documentation/Tests
The changelog fragment is present and the prior counterfactual-identification overstatement is corrected. Path to Approval
|
…instead of routing them to the missing-data fallback The uniform synthetic_weight column for an unbalanced panel is keyed on a non-finite pre-period block, so an infinite outcome took the same branch and was presented as a valid diagnostic (and could leave the ranking scores non-finite). The pre-period pivot is now checked for infinities right after it is built and raises ValueError naming the outcome column; NaN keeps its documented meaning of a missing cell. Parametrized test for an infinite treated and an infinite control outcome. Tutorial 25's tie-sensitivity caveat pointed at "sparse-subset diagnostics below" that the notebook does not run; it now points at the leave-one-out diagnostic below and names sparse_synthetic_control().
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
Methodology
The affected ADH donor-weight and CWZ proxy objectives match their cited sources. Analytical inference remains jointly NaN through Code Quality
PerformanceNo findings. MaintainabilityNo findings. Deprecated parameters were removed consistently from solver and snapshot plumbing. Tech Debt
SecurityNo findings. Documentation/Tests
The changelog fragment is present, and the previous tutorial diagnostic-reference issue is resolved. Path to Approval
|
…he ranking The guard scanned the whole pre-period pivot, so an infinite outcome in a unit listed in exclude_units raised although that unit cannot reach any output. The check now runs after the treated list and the non-excluded control candidates are resolved and covers only those columns, on the valid pre-periods. Test: an excluded control with an infinite outcome neither raises nor appears, and the ranking columns stay finite. Two prep tests still passed the deprecated inner_min_decrease to SyntheticControl and emitted DeprecationWarnings; the kwarg is dropped (the prep suite now runs clean with DeprecationWarning as an error).
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good Executive summary
Methodology
No unmitigated methodology, variance, assumption-check, or inference findings. Code Quality
PerformanceNo findings. MaintainabilityNo findings. Deprecated inner-solver parameters were removed consistently from downstream solver and snapshot plumbing. Tech Debt
SecurityNo findings. Documentation/TestsNo findings. The behavioral changelog fragment is present, and the excluded-infinite-unit regression appears at Test execution was unavailable because this review environment lacks |
…rgence in the warning _sc_weight_nnls reports converged=False either when the active set exhausts its step guard or when the conditioned problem leaves the float64 range (an overflowing column norm or reciprocal, a non-finite intermediate), but the fit-time warning named only the step guard. The warning and the _inner_solve_W, conformal proxy and in_space_placebo docstrings now name both; the fail-closed behavior is unchanged.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good Executive summary
Methodology
Code QualityNo findings. The prior diagnostic issue is fixed at PerformanceNo findings. MaintainabilityNo findings. Deprecated inner-solver parameters were consistently removed from downstream plumbing. Tech Debt
SecurityNo findings. Documentation/TestsNo findings. The behavioral changelog fragment and targeted solver, non-finite-data, and fail-closed tests are present. Static parsing passed; runtime tests were unavailable because NumPy/pytest are not installed. |
…ing, in the opposing-large-donors case test_sc_weight_nnls_opposing_large_donors_beside_a_small_one asserted that the returned weights' objective was no worse than the vertex (0, 1e-8, 1 - 1e-8). Both candidates sit at the rounding floor of the 1e8 entries (objectives 8.1e-17 vs 1.0e-16 on Accelerate, 1.5e-16 vs 1.0e-16 on OpenBLAS), so the ordering is decided by BLAS rounding and the test failed on every x86 CI leg while passing on macOS and arm. The test now asserts what the case was pinned for: a converged simplex vector whose residual is within the rounding of the 1e8 entries (two ulps, since 1e8 * (w1 - w0) carries the rounding of two O(0.5) weights).
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good Executive summary
Methodology
Code QualityNo findings. The prior diagnostic issue is fixed at PerformanceNo findings. The active-set solve removes repeated budget exhaustion without changing the mathematical objective. MaintainabilityNo findings. Deprecated inner-solver parameters are consistently removed from snapshots, CV, placebo, sparse, and conformal paths. Tech Debt
SecurityNo findings. Documentation/TestsNo findings. The changelog fragment covers all user-visible changes, and tests cover KKT conditions, scale conditioning, non-finite inputs, fail-closed behavior, parameter deprecation, and CWZ parity. The latest rounding-floor test remains appropriately platform-independent. Static Python and notebook parsing passed; runtime tests were unavailable because NumPy is not installed. |
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
2 participants
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.
Same-repo mirror of contributor PR #842 (
ilgrad:fix/sc-exact-simplex-weights, fixes #838) so the CI reviewer can run on it. Everything here is pushed back to the contributor's branch once clean; #842 is what gets merged and this PR is closed unmerged.Summary
dd6f4681(contributor, rebased onto main after fix(synthetic_control): put the treated unit in every in-space placebo's donor pool #841):SyntheticControlsolves its donor weights exactly by one non-negative least squares (Lawson-Hanson active set, in-house so the vertex returned at an exact-fit tie does not move with the SciPy release) instead of a budgeted Frank-Wolfe that stopped oninner_max_iteron ordinary factor-model panels, failing the treated fit closed or dropping placebos. The same solve replaces Frank-Wolfe in the CWZ conformal proxy and inrank_control_units' informationalsynthetic_weight. Results move by the former solver's gap (Tutorial 25 is an exact-fit tie: ATT 6.79 -> 6.69, pre-RMSPE 0.987 -> 0.934); REGISTRY records the tie convention as a Deviation from RSynth.82122cad: two cv-infeasibility warnings still namedinner_max_iter; the fix(synthetic_control): put the treated unit in every in-space placebo's donor pool #841 donor-order comment said Frank-Wolfe (the active set also breaks exact gradient ties by position, so the canonicalization stays); Tutorial 25 re-executed on top of fix(synthetic_control): put the treated unit in every in-space placebo's donor pool #841 (constant-effect set [3.47, 20.45], V exactly uniform; the prose quotes neither).ae129c77: the NNLS conditioning scale is computed from the max-normalized difference block, so finite panels of any magnitude give the same weights (a rawsqrt(mean(Z**2))overflowed beyond ~1e154 and let the active set return an arbitrary vertex as converged); a non-finite difference fails closed (uniform,converged=False); the ridge forzeta > 0is applied per passive set instead of a dense J x J identity; the treated-fit non-convergence messages name the inner step guard beside the outer V search and scope the remedy to the latter.fb64bc1c: a ridge-dominated passive set (largelambda_regor a tiny outcome scale inrank_control_units) is solved by its regularized normal equations; the stacked SVD lost the O(1/ridge^2) coefficients (returned [0.4986, 0.5014] as converged where the optimum is [0.5, 0.5]).cafefb13: each donor column is scaled to unit norm inside the reduction (the sum-constraint row carries the inverse norms), so heterogeneous donor scales are solved exactly (a 1e16 donor beside O(1) donors returned a 0.5 residual as converged under a single global scale); the entering column is still chosen on the raw-unit gradient, so the vertex an exact-fit tie returns is unchanged (Tutorial 25 and its drift bands are untouched).6c21a798: regression test pinning objective parity for two nearly opposing 1e8 donors beside an O(1) one (the candidate optima differ by ~1e-17, below the rounding of the data; the returned point is no worse than the alternative vertex and its residual is within one ulp).f6770636:rank_control_units'synthetic_weightdocstring, solver comment and test describe the column as the exact solve thresholded at 1e-6 and renormalized, not the optimum to the last digit.274e35f8: every intermediate of the conditioned problem is checked finite and the active set refuses a non-finite problem, so subnormal or near-overflow data (column norms or their reciprocals outside ~1e-300..1e300) fails closed as a non-converged fit instead of passing a NaN problem as converged; the docs scope the magnitude guarantee to that range.244d85d8:inner_max_iter/inner_min_decreaseare deprecated warn-and-discard no-ops (ledger M-148 / M-149, deprecated 4.0, removed 5.0; migration guide, design-spec phase cell and naming-guard tokens updated) instead of silent no-ops, and the dead budget plumbing is removed from the fit snapshot, the inner/outer solvers and the conformal helpers. A step-guard exhaustion in the active set (50 steps per donor, not reached in practice) returns the feasible iterate flaggedconverged=Falseand takes the existing fail-closed paths instead of raisingRuntimeErrorout of a nested V search or a conformal grid.Validation on this branch (Rust backend, macOS)
scipy.optimize.nnlswhere the optimum is unique; duplicated columns, zero rows, identical donors, J=1 and wide problems handled; identical weights at panel scales from 1e-300 to 1e300 and under a 1e16 within-row dynamic range; subnormal / overflowing inputs fail closed.atol=2e-5(was 1e-3); Tutorial 25 drift bands; 2002 targeted tests across the SC, prep, diagnostic/business-report, base-estimator, ledger, naming-guard, tracking-file, changelog, guide and docs-IA suites.Methodology references (required if estimator / math changes)
Synth: at an exact-fit tie the reportedWis the active set's vertex whereipoptends to an interior point (REGISTRYNote (deviation from R)).Validation
tests/test_methodology_synthetic_control.py(NNLS KKT / vertex / scipy-parity / rounding-guard / step-guard tests, exact inner solve on a factor panel, every placebo kept, conformal proxy converges, deprecation shim, budget-flag propagation, non-convergence paths via a monkeypatched truncation, Basque at 2e-5, CWZ proxy vs SLSQP at 1e-7),tests/test_prep.py(exactsynthetic_weight, uniform on an unbalanced panel),tests/test_t25_synthetic_control_policy_drift.py,tests/test_naming_guard.py,tests/test_diagnostic_report.py,tests/test_business_report.py.Security / privacy
Changelog
changelog.d/20261006-sc-exact-simplex-weights.md: Fixed, Behavioral Changes, Deprecated)