Signal non-convergence in FE imputation alternating-projection solvers by igerber · Pull Request #314 · igerber/diff-diff

igerber · 2026-04-18T13:16:26Z

Summary

Add warn_if_not_converged helper in diff_diff.utils (reusable across axis-B fixes).
Thread a converged flag through the four alternating-projection loops in TwoStageDiD._iterative_fe, TwoStageDiD._iterative_demean, ImputationDiD._iterative_fe, ImputationDiD._iterative_demean; call the helper after the loop.
Pattern matches the existing logistic + Poisson IRLS warnings at linalg.py:1329-1376.
Warning-only: no new public parameter, no behavior change on inputs that already converge.

Methodology references (required if estimator / math changes)

Methods: Borusyak-Jaravel-Spiess (2024) imputation DiD; Gardner (2022) two-stage DiD.
Intentional deviation from R: none. R's didimputation and did2s do not emit a non-convergence signal either, but this library already emits convergence warnings in its other IRLS paths; extending that convention to alternating projection is an additive signal, not a methodology change.
REGISTRY.md updated under both ImputationDiD and TwoStageDiD with **Note:** labels describing the new signal.

Validation

New tests in tests/test_imputation.py::TestImputationEdgeCases (3) and tests/test_two_stage.py::TestSilentWarningAudit (3) exercise non-convergence under tight tol/max_iter and assert the warning fires, plus one negative-control test per estimator.
156 tests pass across test_imputation.py + test_two_stage.py (150 pre-existing + 6 new). Zero regressions.
No tutorial or notebook changes.

Security / privacy

Confirm no secrets/PII in this PR: Yes

TwoStageDiD and ImputationDiD each run two iterative alternating-projection solvers (_iterative_fe, _iterative_demean) whose convergence loop exited silently on max_iter exhaustion, returning the current iterate as if converged. This matches the silent-failure pattern audited under axis B of the silent-failures initiative (findings #2-#5). Adds a shared warn_if_not_converged helper in diff_diff.utils and calls it from all four alternating-projection loops on non-convergence. Pattern mirrors the existing logistic + Poisson IRLS convergence warnings in linalg.py (lines 1329-1376). Warning-only: no new public parameter, no behavior change on inputs that already converge. Updates REGISTRY.md entries for ImputationDiD and TwoStageDiD with Note labels describing the new signal. Axis-B regression-lint baseline: 10 silent range(max_iter) loops -> 6 remaining (Frank-Wolfe and TROP addressed in follow-up PRs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

github-actions · 2026-04-18T13:21:34Z

Overall assessment

✅ Looks good

Executive summary

The affected methods are ImputationDiD and TwoStageDiD; the PR only adds non-convergence signaling around the alternating-projection fixed-effect/demeaning routines, and does not change the underlying estimator definitions or variance formulas described in the source material. (academic.oup.com)
The methodology change is explicitly documented as a **Note:** for both estimators in docs/methodology/REGISTRY.md:L1086 and docs/methodology/REGISTRY.md:L1164, so this is not an undocumented deviation.
Within diff scope, the pattern fix is complete: all four alternating-projection loops now thread converged and call the shared helper in diff_diff/imputation.py:L872-L1012, diff_diff/two_stage.py:L855-L984, and diff_diff/utils.py:L68-L88.
I found no unmitigated P0 or P1 issues.
Minor test gap: _iterative_demean has a non-convergence test, but no convergent-path negative control in either estimator: tests/test_imputation.py:L2119-L2129 and tests/test_two_stage.py:L1356-L1366.

Methodology

Severity: P3 informational. Impact: no methodology defect. The warning is additive only; the affected routines still implement untreated-observation FE estimation / residualization as required by the Borusyak-Jaravel-Spiess imputation estimator and Gardner’s two-stage estimator, and the registry documents the new behavior in-note. Concrete fix: none. See diff_diff/imputation.py:L872-L1012, diff_diff/two_stage.py:L855-L984, docs/methodology/REGISTRY.md:L1086, and docs/methodology/REGISTRY.md:L1164. (academic.oup.com)

Code Quality

Severity: none. Impact: no code-quality defects identified in the changed lines; extracting diff_diff/utils.py:L68-L88 removes duplication cleanly. Concrete fix: none.

Performance

Severity: none. Impact: no meaningful performance risk; the added boolean flag and post-loop helper call are negligible relative to the iterative groupby passes. Concrete fix: none.

Maintainability

Severity: none. Impact: no maintainability issue identified; the shared helper makes future convergence-warning changes less error-prone across estimators. Concrete fix: none.

Tech Debt

Severity: none. Impact: no new deferred work needs TODO.md tracking from this diff. Concrete fix: none.

Security

Severity: none. Impact: no security issues identified; the PR only adds warning plumbing, docs, and tests. Concrete fix: none.

Documentation/Tests

Severity: P3. Impact: _iterative_demean now has a positive warning test, but there is no matching convergent negative-control test in either estimator, so an accidental always-warn regression on the demean path would not be caught. Concrete fix: add warnings.catch_warnings(record=True) tests mirroring the FE negative control for _iterative_demean in tests/test_imputation.py:L2119-L2129 and tests/test_two_stage.py:L1356-L1366.

Review note: this was a static review only. I could not execute the tests in this environment because pytest and numpy are unavailable.

Mirrors the positive + negative coverage already present for _iterative_fe. Ensures an accidental always-warn regression on the alternating-projection demean path would fail the test suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

igerber · 2026-04-18T13:24:17Z

/ai-review

github-actions · 2026-04-18T13:34:24Z

🔁 AI review rerun (requested by @igerber)

Head SHA: 5fcb63d375b55a47d5c527ed46b7f554914ee363

Overall Assessment

Needs changes.

Executive Summary

Re-review scope: the prior P3 about missing convergent negative-control coverage for _iterative_demean is addressed by the new no-warning tests in tests/test_imputation.py:L2131 and tests/test_two_stage.py:L1368.
The affected methods are ImputationDiD and TwoStageDiD. The new warning hooks are confined to the untreated-observation FE / residualization solvers and are documented in docs/methodology/REGISTRY.md:L1086 and docs/methodology/REGISTRY.md:L1164; they do not change the source-material estimator equations or variance formulas. (academic.oup.com)
Severity P1 [Newly identified]: the same silent non-convergence pattern still exists in the weighted alternating-projection loops inside diff_diff/utils.py:L1865, even though this PR adds a shared warning helper in the same file.
That gap is live in production code: diff_diff/wooldridge.py:L651 always passes a weight vector into diff_diff/wooldridge.py:L667, so Wooldridge’s within-transformation still has the silent-failure pattern this PR is standardizing away.
No P0 findings.
Static review only: I could not execute pytest here because pytest is not installed.

Methodology

Severity: P3 informational. Impact: the changed estimator paths are the alternating-projection FE / demeaning routines in diff_diff/imputation.py:L872, diff_diff/imputation.py:L956, diff_diff/two_stage.py:L855, and diff_diff/two_stage.py:L933. Borusyak-Jaravel-Spiess derive the imputation-form estimator, and Gardner’s two-stage procedure identifies fixed effects from untreated observations before estimating treatment effects on residualized outcomes; this PR only adds post-loop signaling and leaves those method definitions intact. The additive behavior change is documented in the Methodology Registry notes cited above. Concrete fix: none. (academic.oup.com)

Code Quality

Severity: P1 [Newly identified]. Impact: this PR fixes the silent non-convergence pattern in four alternating-projection loops, but diff_diff/utils.py:L1882 and diff_diff/utils.py:L1893 still run weighted alternating projections up to a hard-coded 100 iterations and then silently return the last iterate with no warn_if_not_converged() call. That is exactly the checklist’s “partial pattern fix” case. Because diff_diff/wooldridge.py:L651 supplies either survey weights or np.ones(...) and diff_diff/wooldridge.py:L667 routes through within_transform(), the unresolved silent-failure path is not dead code. Concrete fix: add converged/for...else handling to both weighted loops in within_transform() using diff_diff/utils.py:L68, or refactor those loops into a shared private helper that emits the warning on exhaustion.

Performance

No findings.

Maintainability

No findings. The shared helper in diff_diff/utils.py:L68 is a maintainability improvement for the four modified estimator loops.

Tech Debt

No separate finding. TODO.md:L51 does not already track the within_transform() silent non-convergence path, so the P1 above is unmitigated.

Security

No findings.

Documentation/Tests

No unmitigated findings. The prior re-review concern about missing convergent _iterative_demean negative controls is resolved by tests/test_imputation.py:L2131 and tests/test_two_stage.py:L1368. I could not run the tests in this environment because pytest is unavailable.

Path to Approval

Apply the same non-convergence warning pattern to the weighted alternating-projection loops in diff_diff/utils.py:L1882 and diff_diff/utils.py:L1893, so exhausting the hard-coded 100 iterations is no longer silent.
Add regression tests for that weighted path: one forced non-convergence case that asserts the warning, and one convergent negative control. Covering the production caller via diff_diff/wooldridge.py:L667 would be sufficient.

…ction AI review on PR #314 surfaced two more silent alternating-projection loops in diff_diff.utils.within_transform (previously hard-coded range(100) with tol=1e-8). The weighted path is exercised on every WooldridgeDiD fit via wooldridge.py:667, so leaving it silent would have left this PR as a "partial pattern fix" for axis B. - Expose max_iter and tol as optional kwargs (defaults preserved: 100, 1e-8). - Track non-converged variables across the per-variable inner loops and emit a single consolidated UserWarning naming which variables failed, reusing the warn_if_not_converged helper already added in this PR. - Add a positive warning test and a convergent negative control in TestWithinTransformationAlgebra. Axis-B regression-lint baseline: 10 -> 4 silent range(max_iter) loops remaining (Frank-Wolfe, TROP global outer+inner, TROP local). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

igerber · 2026-04-18T14:00:34Z

/ai-review

github-actions · 2026-04-18T14:06:09Z

🔁 AI review rerun (requested by @igerber)

Head SHA: 4a7fee3785b3a59f6cde665bc289e5a8c9b9070f

Overall Assessment

✅ Looks good

Executive Summary

Re-review scope: the prior P1 on the still-silent weighted alternating-projection path is addressed. within_transform() now tracks per-variable convergence and warns on exhaustion in diff_diff/utils.py:L1881-L1919, covering the production path used from diff_diff/wooldridge.py:L647-L670.
The methodology-affected estimators are ImputationDiD and TwoStageDiD. The changed paths in diff_diff/imputation.py:L872-L1012 and diff_diff/two_stage.py:L855-L985 add only post-loop signaling; they do not change the Borusyak-Jaravel-Spiess or Gardner estimating equations, weighting, identification logic, or variance formulas. The new warning behavior is documented in docs/methodology/REGISTRY.md:L1068-L1086 and docs/methodology/REGISTRY.md:L1152-L1164.
I found no unmitigated P0/P1 issues.
Minor P3 only: within_transform() gained public max_iter/tol parameters, but its docstring does not document them in the Parameters block diff_diff/utils.py:L1809-L1846.
Static review only: I could not run the new tests here because pytest is not installed in this environment.

Methodology

No findings. The new warn_if_not_converged() hooks in diff_diff/imputation.py:L872-L1012 and diff_diff/two_stage.py:L855-L985 are warning-only and consistent with the registry notes in docs/methodology/REGISTRY.md:L1068-L1086 and docs/methodology/REGISTRY.md:L1152-L1164.

Code Quality

No findings. The previous re-review P1 is resolved by the weighted within_transform() changes in diff_diff/utils.py:L1881-L1919, which remove the remaining silent exhaustion path reached from diff_diff/wooldridge.py:L647-L670.

Performance

No findings.

Maintainability

No findings. Centralizing warning emission in diff_diff/utils.py:L68-L88 reduces duplicated convergence-warning code across the estimator loops and the weighted utility path.

Tech Debt

No findings.

Security

No findings.

Documentation/Tests

Severity: P3. Impact: within_transform() now exposes max_iter and tol in its public signature diff_diff/utils.py:L1809-L1818, but the docstring parameters list still stops at weights diff_diff/utils.py:L1829-L1846. That leaves the new tuning knobs undocumented and does not tell users they only affect the weighted alternating-projection branch. Concrete fix: add max_iter and tol to the docstring (and generated API docs, if applicable), explicitly noting that they are used only when weights is not None.
No other findings. The PR adds the expected warning/no-warning coverage in tests/test_imputation.py:L2091-L2143, tests/test_two_stage.py:L1328-L1380, and tests/test_methodology_twfe.py:L238-L257, but I could not execute them locally because pytest is unavailable.

Both parameters apply only when weights is not None. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

igerber · 2026-04-18T14:07:52Z

/ai-review

github-actions · 2026-04-18T14:13:27Z

🔁 AI review rerun (requested by @igerber)

Head SHA: ef388a044463d3c7e4a333a74f6cc1192b9f41e4

Overall Assessment

✅ Looks good

Executive Summary

Re-review scope: the prior documentation gap on within_transform(max_iter, tol) is resolved in diff_diff/utils.py:L1816.
Methodology-affected estimators are ImputationDiD and TwoStageDiD. The new warn_if_not_converged() calls are post-loop only in diff_diff/imputation.py:L872 and diff_diff/two_stage.py:L855; they do not change the BJS/Gardner estimating equations, weighting, identification logic, or variance formulas.
The warning-only behavior is documented in the Methodology Registry for those two estimators at docs/methodology/REGISTRY.md:L1086 and docs/methodology/REGISTRY.md:L1164.
No unmitigated P0/P1 findings.
P3 only: the shared within_transform() warning path also affects WooldridgeDiD and survey-weighted TWFE, SunAbraham, and BaconDecomposition, but the registry updates only describe ImputationDiD and TwoStageDiD.
Static review only: I could not execute the new tests because pytest is not installed in this environment.

Methodology

No findings. The alternating-projection updates themselves are unchanged; only post-exhaustion signaling was added in diff_diff/imputation.py:L912, diff_diff/imputation.py:L984, diff_diff/two_stage.py:L890, and diff_diff/two_stage.py:L957. That is consistent with the cited methods and the new registry notes.

Code Quality

No findings. Centralizing the warning emission in diff_diff/utils.py:L68 removes duplicated warning text and keeps the silent-failure fix consistent across the touched loops.

Performance

No findings. The added work is a constant-time post-loop check and warning call.

Maintainability

No findings.

Tech Debt

No findings. I did not see a new silent-correctness issue that would need TODO tracking.

Security

No findings.

Documentation/Tests

Severity: P3. Impact: the shared weighted alternating-projection path in diff_diff/utils.py:L1809 now warns on non-convergence, and that path is used directly by diff_diff/wooldridge.py:L667 and by survey-weighted diff_diff/twfe.py:L151, diff_diff/sun_abraham.py:L979, and diff_diff/bacon.py:L656. The registry updates at docs/methodology/REGISTRY.md:L1086 and docs/methodology/REGISTRY.md:L1164 only document ImputationDiD and TwoStageDiD, so the affected-method inventory is incomplete for this warning-only behavior. Concrete fix: add either a shared registry note near docs/methodology/REGISTRY.md:L2514 covering weighted within-transformation warnings, or per-estimator notes at least where WooldridgeDiD already documents iterative alternating projection in docs/methodology/REGISTRY.md:L1303.
No other findings. The added tests in tests/test_imputation.py:L2091, tests/test_two_stage.py:L1328, and tests/test_methodology_twfe.py:L238 cover the intended warning/no-warning behavior, but I could not run them locally because pytest is unavailable.

…tors AI review surfaced that the shared within_transform weighted path also affects WooldridgeDiD and survey-weighted TwoWayFixedEffects, SunAbraham, and BaconDecomposition, but the prior notes only covered ImputationDiD and TwoStageDiD. Adds one shared note under 'Absorbed Fixed Effects with Survey Weights' covering the cross-estimator behavior, plus a per-estimator note in the WooldridgeDiD section (since its path always routes through the weighted helper, regardless of whether survey weights are provided). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

igerber · 2026-04-18T14:15:46Z

/ai-review

github-actions · 2026-04-18T14:29:38Z

🔁 AI review rerun (requested by @igerber)

Head SHA: 48622a3fb01357d825e4645d9dc61b6ac8f76b1f

Overall Assessment

✅ Looks good

Executive Summary

Re-review scope: the prior P3 documentation gap around shared weighted within_transform() consumers appears resolved by the new absorbed-FE registry note and the Wooldridge-specific note at docs/methodology/REGISTRY.md:2525 and docs/methodology/REGISTRY.md:1304.
Methodology-affected paths are warning-only post-loop checks in ImputationDiD and TwoStageDiD, plus the shared weighted within_transform() path at diff_diff/imputation.py:949, diff_diff/imputation.py:1011, diff_diff/two_stage.py:926, diff_diff/two_stage.py:984, and diff_diff/utils.py:1922.
I did not find any change to estimating equations, control/comparison-group logic, weighting, identification assumptions, or variance/SE computation.
No unmitigated P0/P1 findings.
One P3 informational item remains: the estimator registry notes document the iterative FE warning but not the newly added iterative demeaning warning paths.
Static review only: pytest is not installed here, and import-time runtime deps are missing, so I could not execute the added tests.

Methodology

No findings. The changes are post-loop warning emissions only; the BJS/Gardner estimator definitions and inference codepaths are unchanged in diff_diff/imputation.py:912, diff_diff/two_stage.py:890, docs/methodology/REGISTRY.md:1086, and docs/methodology/REGISTRY.md:1164.

Code Quality

No findings. Centralizing the warning text in diff_diff/utils.py:68 keeps the new signal consistent across the touched alternating-projection loops.

Performance

No findings. The added work is a constant-time convergence-flag check and an optional warning after the loop.

Maintainability

No findings. The shared-helper documentation coverage is materially better now that the absorbed-FE registry note explicitly names TwoWayFixedEffects, SunAbraham, BaconDecomposition, and WooldridgeDiD at docs/methodology/REGISTRY.md:2529.

Tech Debt

No findings. I did not identify a new correctness issue that should be deferred into TODO.md.

Security

No findings.

Documentation/Tests

Severity: P3. Impact: the estimator-specific registry notes still say only the iterative FE solver warns, but this PR also adds warnings to _iterative_demean in both estimators at diff_diff/imputation.py:1011 and diff_diff/two_stage.py:984. Those demeaning helpers are used in covariate residualization at diff_diff/imputation.py:1138 and diff_diff/two_stage.py:1029, and in ImputationDiD pre-trend demeaning at diff_diff/imputation.py:2077. Concrete fix: broaden the notes at docs/methodology/REGISTRY.md:1086 and docs/methodology/REGISTRY.md:1164 to cover both iterative FE and iterative demeaning, or add separate _iterative_demean notes.
Residual risk: I could not run tests/test_imputation.py:2091, tests/test_two_stage.py:1328, or tests/test_methodology_twfe.py:238 because pytest is unavailable in this environment.

…mean The warning behavior added in this PR also fires from the iterative demeaning helper (covariate residualization in both estimators, plus the ImputationDiD pre-trend test). Earlier notes named only the FE solver. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

igerber · 2026-04-18T14:31:50Z

/ai-review

github-actions · 2026-04-18T14:36:22Z

🔁 AI review rerun (requested by @igerber)

Head SHA: f74941661a3d116fe2f37c713d6214024be1b7df

Overall Assessment

✅ Looks good

No unmitigated P0/P1 findings.

Executive Summary

Re-review scope: the prior P3 documentation gap is resolved. REGISTRY.md now documents non-convergence warnings for ImputationDiD and TwoStageDiD demeaning paths at docs/methodology/REGISTRY.md:1086, docs/methodology/REGISTRY.md:1164, plus the shared weighted within_transform note at docs/methodology/REGISTRY.md:1304 and docs/methodology/REGISTRY.md:2525.
The methodology-affected code is warning-only: post-loop convergence checks were added to diff_diff/imputation.py:949, diff_diff/imputation.py:1011, diff_diff/two_stage.py:926, diff_diff/two_stage.py:984, with the shared helper in diff_diff/utils.py:68 and the weighted within_transform warning at diff_diff/utils.py:1922.
I did not find any change to estimating equations, control/comparison-group logic, weighting formulas, variance/SE computation, identification assumptions, or default behavior on already-convergent inputs.
The shared weighted-path documentation matches actual callers in diff_diff/twfe.py:151, diff_diff/sun_abraham.py:979, diff_diff/bacon.py:656, and diff_diff/wooldridge.py:667.
Added tests cover both warning and no-warning branches for the newly instrumented solvers in tests/test_imputation.py:2091, tests/test_two_stage.py:1338, and tests/test_methodology_twfe.py:238.
Static review only: I could not run the tests here because pytest, numpy, and pandas are not installed in this environment.

Methodology

No findings. The affected methods are Borusyak-Jaravel-Spiess imputation DiD and Gardner two-stage DiD, but the diff only adds post-loop warning emission after existing convergence checks in diff_diff/imputation.py:949, diff_diff/imputation.py:1011, diff_diff/two_stage.py:926, and diff_diff/two_stage.py:984. The new signal is explicitly documented with **Note:** labels in docs/methodology/REGISTRY.md:1086, docs/methodology/REGISTRY.md:1164, docs/methodology/REGISTRY.md:1304, and docs/methodology/REGISTRY.md:2525, so this is not an undocumented deviation from the source material.

Code Quality

No findings. Centralizing the message in diff_diff/utils.py:68 keeps the warning behavior consistent with the existing non-convergence signaling pattern in diff_diff/linalg.py:1370 and diff_diff/linalg.py:2525, while leaving estimator math untouched.

Performance

No findings. The change adds only constant-time convergence bookkeeping and optional warning emission after loop completion; it does not add iterations, change solvers, or alter asymptotic cost.

Maintainability

No findings. The shared helper removes duplicated warning strings, and the registry updates now cover all touched warning paths, including the previously missing _iterative_demean notes.

Tech Debt

No findings. I did not identify a new silent-correctness issue or a deferrable gap that needs a TODO.md entry.

Security

No findings. The diff is limited to warning emission, docs, and tests; there is no new secrets surface, external I/O, or trust-boundary change.

Documentation/Tests

No findings. The previous documentation gap is addressed, and the new tests cover both positive and negative controls for warning emission in the touched code paths.
Residual risk: static review only. I could not execute tests/test_imputation.py:2091, tests/test_two_stage.py:1338, or tests/test_methodology_twfe.py:238 because this environment lacks pytest, numpy, and pandas.

Absorbs #312 (sdid scale fix), #313 (roadmap refresh + dCDH docstring rewrite), and #314 (within_transform convergence warnings) from main. Conflicts resolved in: - diff_diff/chaisemartin_dhaultfoeuille.py: took main's comprehensive Phase 1-3 feature list in the class docstring but merged in the PR #311 group-vs-PSU bootstrap-clustering framing and the replicate-weight survey-support line. Kept the PR #311 'user-specified cluster= not supported + automatic PSU-level under survey_design' wording for the cluster= parameter docstring (strictly more accurate than main's 'always clusters at the group level' text). - diff_diff/guides/llms-full.txt: kept main's more detailed placebo SE contract paragraph (which already distinguishes single-period NaN from multi-horizon analytical/bootstrap) and appended the sup-t / shared-weights / cross-horizon coverage details from the PR #311 update. Kept the PR #311 survey_design signature comment that mentions TSL + replicate + PSU bootstrap. Full regression across touched areas: 336 + 324 passing.

… remove deprecated SyntheticDiD params Package four merged PRs (#312 SDID catastrophic cancellation at extreme Y scale, #313 roadmap refresh, #314 FE imputation non-convergence signaling, #315 Frank-Wolfe SC weight solver non-convergence signaling) as 3.1.2. Also remove the SyntheticDiD(lambda_reg=...) and SyntheticDiD(zeta=...) kwargs, which have been deprecated with DeprecationWarning since v2.3.1 (2026-02-10) in favor of zeta_omega / zeta_lambda; their warning messages announced removal in v3.1. Passing the old kwargs now raises TypeError at __init__ and ValueError: Unknown parameter at set_params. Internal ridge-regression helpers that accept a lambda_reg parameter (compute_synthetic_weights, rank_control_units, Rust FFI bindings) are unaffected. Version strings bumped in diff_diff/__init__.py, pyproject.toml, rust/Cargo.toml, and diff_diff/guides/llms-full.txt. CHANGELOG populated with Fixed / Changed / Removed sections and comparison-link footer. TODO.md's "Deprecated Code" entry removed now that the task is done. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Per CI review feedback (#316): removing public kwargs under a patch version violates Semantic Versioning, which CHANGELOG.md explicitly claims to adhere to. Restore lambda_reg and zeta handling in SyntheticDiD.__init__ and set_params as warning-only, and bump the removal target in the DeprecationWarning text from "v3.1" to "v4.0.0". The 3.1.2 release now carries only the four fix/doc PRs (#312 SDID scale, #313 roadmap, #314 FE imputation convergence, #315 Frank-Wolfe convergence) with no breaking changes. - diff_diff/synthetic_did.py: restore deprecated kwargs + warnings (v4.0.0 text) - tests/test_methodology_sdid.py: restore TestDeprecatedParams class + set_params deprecation test - tests/test_estimators.py: restore test_deprecated_params - CHANGELOG.md: drop Removed section; add Changed entry documenting the v3.1 -> v4.0.0 bump in the removal target - TODO.md: restore Deprecated Code section with v4.0.0 removal target and SemVer rationale Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

CI review on #322 flagged that the 3.1.3 entries for PR #319 (sparse->dense lstsq fallback) and PR #317 (TROP non-convergence) claimed ConvergenceWarning, but the actual implementations emit UserWarning (imputation.py, two_stage.py, utils.py, trop.py, and the REGISTRY.md contract all use UserWarning). Users filtering warnings by category would be misled. Same factual error was in the 3.1.2 entries I wrote in PR #316 for PR #314 and PR #315. Fixing both entries in this PR — CHANGELOG is a living doc and the warning-category drift is actionable-ly misleading. No code or test changes; CHANGELOG-only edit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Document max_iter and tol parameters in within_transform docstring

ef388a0

Both parameters apply only when weights is not None. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

igerber added the ready-for-ci Triggers CI test workflows label Apr 18, 2026

igerber merged commit 44e7522 into main Apr 18, 2026
23 of 24 checks passed

igerber deleted the fix/fe-imputation-convergence-warnings branch April 18, 2026 15:31

igerber mentioned this pull request Apr 18, 2026

Signal non-convergence in Frank-Wolfe SC weight solver (numpy path) #315

Merged

This was referenced Apr 18, 2026

Release 3.1.2: SDID scale fix, convergence warnings, roadmap refresh #316

Merged

Signal non-convergence in TROP alternating-minimization solvers #317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signal non-convergence in FE imputation alternating-projection solvers#314

Signal non-convergence in FE imputation alternating-projection solvers#314
igerber merged 6 commits intomainfrom
fix/fe-imputation-convergence-warnings

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

igerber commented Apr 18, 2026

Summary

Methodology references (required if estimator / math changes)

Validation

Security / privacy

Uh oh!

github-actions Bot commented Apr 18, 2026

Overall assessment

Executive summary

Methodology

Code Quality

Performance

Maintainability

Tech Debt

Security

Documentation/Tests

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

igerber commented Apr 18, 2026

Uh oh!

github-actions Bot commented Apr 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant