Skip to content

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

Merged
igerber merged 2 commits intomainfrom
fix/fw-convergence-warning
Apr 18, 2026
Merged

Signal non-convergence in Frank-Wolfe SC weight solver (numpy path)#315
igerber merged 2 commits intomainfrom
fix/fw-convergence-warning

Conversation

@igerber
Copy link
Copy Markdown
Owner

@igerber igerber commented Apr 18, 2026

Summary

  • _sc_weight_fw_numpy at utils.py:1434 now threads a converged flag through its Frank-Wolfe loop and calls the shared warn_if_not_converged helper (introduced in Signal non-convergence in FE imputation alternating-projection solvers #314) when max_iter exhausts without hitting the min_decrease^2 gate.
  • Scope: numpy fallback path only. The Rust-backend path (_sc_weight_fw dispatching into rust/src/weights.rs) currently returns weights without convergence status, so threading the signal there requires a cross-language FFI signature change. Left as an axis-G backend-parity follow-up; the Rust path is silent, but it is numerically equivalent to the numpy path and only reached when HAS_RUST_BACKEND=True.
  • Warning-only: no new public parameter; weights on converging inputs are unchanged.

Methodology references

  • Method: Arkhangelsky et al. (2021) Frank-Wolfe-on-the-simplex step for SDID unit and time weights. The solver itself is unchanged.
  • REGISTRY updated: the prior edge-case entry at docs/methodology/REGISTRY.md:1499 explicitly documented "No warning emitted" on non-convergence. This PR updates that entry to reflect the new numpy-path signal and notes the remaining Rust-path gap.

Validation

  • New tests in tests/test_methodology_sdid.py::TestFrankWolfe: one positive test forcing non-convergence (max_iter=1, min_decrease=1e-12) and one convergent negative control (max_iter=10000, min_decrease=1e-3) asserting no warning fires.
  • Full tests/test_methodology_sdid.py regression: 127 passed, 0 failed.
  • No tutorial / notebook changes.

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

@igerber @claude
Signal non-convergence in Frank-Wolfe SC weight solver (numpy path)
c267de6 
_sc_weight_fw_numpy ran its iterative Frank-Wolfe loop up to max_iter
(R's default: 10000) and silently returned the final iterate when the
convergence check vals[t-1] - vals[t] < min_decrease^2 never triggered
early exit. This matches the silent-failure pattern audited under axis
B of the silent-failures initiative (finding #1); REGISTRY:1499
previously documented this as "No warning emitted".

Adds a converged flag to the numpy path and calls the shared
warn_if_not_converged helper on exhaustion. Updates the REGISTRY entry
to describe the new signal. Rust-backend path is unchanged; the
Rust FFI function signature currently returns weights only and would
need to thread a convergence status — left as an axis-G backend-parity
follow-up (tracked in the Phase 2 findings).

Warning-only: no new public parameter, no change to returned weights on
inputs that already converge.

Axis-B regression-lint baseline: 6 -> 5 silent range(max_iter) loops
remaining (TROP global outer + inner + local).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 18, 2026

Overall assessment

✅ Looks good

Highest unmitigated severity: P3. This PR does not change the Arkhangelsky et al. (2021) / synthdid::sc.weight.fw objective, Frank-Wolfe update, weighting, or any variance/SE path; it adds non-convergence signaling on the NumPy fallback and updates the methodology registry accordingly.

Executive summary

  • The affected method is the SDID Frank-Wolfe simplex solver used for unit and time weights; the numerical method itself is unchanged.
  • The new converged flag in the NumPy path is consistent with the shared non-convergence warning pattern already used elsewhere in utils.py.
  • The remaining Rust-backend silence is user-visible because _sc_weight_fw() dispatches to Rust when available, but that divergence is now documented in the methodology registry, so it is P3 informational rather than blocking.
  • The new tests cover the private NumPy helper’s warn/no-warn branches, but they do not yet protect the public wrapper/backend-selection path.
  • No P0/P1 methodology, inference, control-group, NaN-propagation, or security issues were introduced by this diff.

Methodology

Code Quality

  • No findings.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • No separate findings beyond the documented Rust-backend parity follow-up above.

Security

  • No findings.

Documentation/Tests

  • Severity: P3. Impact: the regression tests validate the private NumPy helper directly, but they do not exercise the public wrapper path that callers use in pure-Python mode, and they do not pin the newly relevant max_iter=0 exhaust-without-convergence behavior. That leaves the warning contract easier to regress in future refactors without failing tests. References: tests/test_methodology_sdid.py:L184-L197, tests/test_methodology_sdid.py:L211-L227, diff_diff/utils.py:L1378-L1471. Concrete fix: add wrapper-level tests that patch diff_diff.utils.HAS_RUST_BACKEND = False and assert _sc_weight_fw(...) warns on non-convergence (max_iter=0 and max_iter=1) and stays silent on a convergent case; once Rust exposes convergence status, add a backend-parity test for the Rust path too.

@igerber @claude
Add wrapper-level and max_iter=0 regression tests for FW convergence …
83f395a 
…warning

AI review on PR #315 asked for test coverage of the public _sc_weight_fw
wrapper (rather than only the private _sc_weight_fw_numpy helper), to pin
the warning contract against refactors that would bypass the numpy path.

- test_fw_wrapper_warns_on_nonconvergence_without_rust: patches
  HAS_RUST_BACKEND=False and asserts the wrapper routes through the
  warning on non-convergence.
- test_fw_wrapper_no_warning_on_convergence_without_rust: wrapper-level
  negative control.
- test_fw_max_iter_zero_warns: pins the convention that max_iter=0
  (which returns the uniform init without iterating) emits the warning,
  since by construction no convergence was reached.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@igerber
Copy link
Copy Markdown
Owner Author

igerber commented Apr 18, 2026

/ai-review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 18, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: 83f395ac2760691d772398abd05c656062ab9b61

Overall Assessment

✅ Looks good

No unmitigated P0/P1 findings. The affected method is SDID’s Frank-Wolfe solver for unit and time weights; this diff leaves the solver math intact and only adds non-convergence signaling on the NumPy fallback. The official SDID paper establishes the estimator, and the official synthdid solver source uses the same penalized simplex objective and fw.step/max.iter loop without a built-in warning channel, so this PR is a documented signaling change rather than a methodology change. diff_diff/utils.py:L1388 diff_diff/utils.py:L1471 (aeaweb.org)

Executive Summary

  • The prior re-review P3 is addressed: wrapper-level coverage now exists for _sc_weight_fw(...) with HAS_RUST_BACKEND=False, and the previously missing max_iter=0 exhaust-without-convergence case is pinned. tests/test_methodology_sdid.py:L229 tests/test_methodology_sdid.py:L252
  • The numerical method itself is unchanged; the diff only threads a converged flag and calls the shared warn_if_not_converged(...) helper after the existing loop. diff_diff/utils.py:L1463 diff_diff/utils.py:L1471 (rdrr.io)
  • The Rust dispatch path still returns silently because _sc_weight_fw() hands control straight to _rust_sc_weight_fw(...), but that divergence is explicitly documented in the methodology registry and is therefore P3 informational, not blocking. diff_diff/utils.py:L1416 docs/methodology/REGISTRY.md:L1499
  • Verification note: I could not execute the test file in this environment because pytest and numpy are unavailable here.

Methodology

  • Severity: P3. Impact: _sc_weight_fw() still has backend-dependent warning semantics: the NumPy path warns on exhaustion, while the Rust path still returns the final iterate silently because the wrapper cannot receive a convergence flag back from _rust_sc_weight_fw(...). This does not change the Arkhangelsky et al. SDID estimator or the sc.weight.fw objective/step itself, and the deviation is explicitly documented in the registry, so it is informational only. Concrete fix: when the Rust FFI can return convergence status, thread that boolean through _sc_weight_fw() and call the same helper for both backends. diff_diff/utils.py:L1416 diff_diff/utils.py:L1434 diff_diff/utils.py:L1471 docs/methodology/REGISTRY.md:L1499 (aeaweb.org)

Code Quality

  • No findings.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

Security

  • No findings.

Documentation/Tests

@igerber igerber added the ready-for-ci Triggers CI test workflows label Apr 18, 2026
@igerber igerber merged commit 32a4d09 into main Apr 18, 2026
23 of 24 checks passed
@igerber igerber deleted the fix/fw-convergence-warning branch April 18, 2026 17:27
igerber added a commit that referenced this pull request Apr 18, 2026
@igerber @claude
Release 3.1.2: SDID scale fix, convergence warnings, roadmap refresh;…
231dca9 
… 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>
@igerber igerber mentioned this pull request Apr 18, 2026
Merged
igerber added a commit that referenced this pull request Apr 18, 2026
@igerber @claude
Revert SyntheticDiD deprecation removal; keep warning-only through 3.x
9230c8f 
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>
igerber added a commit that referenced this pull request Apr 18, 2026
@igerber @claude
Fix CI review: CHANGELOG warning category mismatched shipped code
725c452 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@igerber