Add proximal L1 solver path - #184
Conversation
MaxGhenis
left a comment
There was a problem hiding this comment.
Review: PolicyEngine/populace PR #184
Recommendation: REQUEST_CHANGES
Findings
- [CRITICAL] The L1 path does not implement the objective it documents and records.
In packages/populace-calibrate/src/populace/calibrate/solve.py, _optimize_proximal documents the objective as capped weighted-MAPE plus l1_lambda * mean(w_i / w0_i), and result metadata records l1_penalty="mean_initial_weight_ratio_abs". But the update at lines 685-690 normalizes the smooth gradient step by RMS, making the effective step size learning_rate / rms, while the soft-threshold is fixed at learning_rate * l1_lambda. For the documented mean penalty, the threshold should use the same effective step size and the mean divisor, approximately (learning_rate / rms) * l1_lambda / n. The current update is therefore not the proximal operator for the recorded objective; it behaves like an unnormalized heuristic whose lambda scale changes with record count and gradient scale. That is especially risky because this PR is meant to supply a solver/provenance path for l0-paper comparisons.
Relevant lines: packages/populace-calibrate/src/populace/calibrate/solve.py:627, :632, :648, :652, :685, :690, :1289.
Suggested fix: choose one contract and make code, docs, metadata, and tests agree. If the contract is mean(w/w0), use the actual effective step in the prox shrink and divide by n. If the desired behavior is a budget heuristic, rename the metadata away from an objective penalty and stop claiming it minimizes target_loss + lambda * mean(r).
Test Gap
The new tests show that prox can produce exact zeros and that pruning is monotone on one fixture. They do not test the L1 scale/objective contract. Add a small deterministic one-step prox test or a duplicate-record invariance test for the declared mean penalty.
Other Notes
The PR is mergeable but the branch is three commits behind the current #182 base. It should be rebased after the solver issue is fixed. No GitHub checks were reported for this branch during review; local focused test_solve.py passed 37 tests.
42c1a58 to
258e7eb
Compare
|
Fixed the review finding and pushed the update to #184. What changed:
Verification:
|
There was a problem hiding this comment.
Follow-up after the pushed fix: the L1 proximal update now matches the documented objective, l1_lambda * mean(w / w0), because the shrink threshold uses the same effective smooth step size and divides by record count. The new one-step regression test covers the scale contract that the original review flagged.
Validated locally with pytest packages/populace-calibrate/tests/test_solve.py -q and pytest packages/populace-calibrate/tests -q. I am leaving this as a comment review rather than approving my own pushed fix.
MaxGhenis
left a comment
There was a problem hiding this comment.
Follow-up after the second review pass: I pushed 99b4f36 to address the remaining solver-contract issues. method="prox" now rejects l2_lambda > 0 instead of recording an L2 objective it does not optimize, and the L1 scale tests now cover both the zero-gradient / n shrink and a nonzero-gradient case that pins use of the RMS-normalized effective step.
Validated with uv run --package populace-calibrate --group dev python -m pytest packages/populace-calibrate/tests/test_solve.py -q, uv run --package populace-calibrate --group dev python -m pytest packages/populace-calibrate/tests -q, uv run --no-sync ruff check packages/populace-calibrate/src/populace/calibrate/solve.py packages/populace-calibrate/tests/test_solve.py, and git diff --check. Existing torch sparse warnings only.
b440c33 to
a56aefd
Compare
The method arg accepted "apg"/"adam" but both ran torch Adam on log-weights, so manifests could record method="apg" for an Adam run. Adam also cannot perform the soft-thresholding L1 needs. This adds method="prox" for proximal gradient on weight ratios r=w/w0, rejects the misleading apg alias, and guards l1_lambda so it can only be used with the prox path. The L1 contract is explicit: l1_lambda multiplies mean(w/w0). The prox shrink uses the same effective smooth-step size and divides by n, so the soft-threshold matches the recorded mean-initial-weight-ratio objective rather than an unnormalized sum penalty. Tests cover sparse selection, lambda monotonicity, apg rejection, prox-only l1_lambda, and a one-step zero-gradient case that pins the mean-ratio prox scale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
99b4f36 to
8bbf118
Compare
Summary
Adds a real proximal L1 selection path to
populace-calibratewhile preserving compatibility with existing solver configs.method="prox"withl1_lambda, optimizing non-negative weight ratios and applying an L1 soft-threshold step so records can be selected to exact zero.method="adam".method="apg"as a deprecated alias for"adam"; old configs still run, but result options normalize to"adam"so new manifests do not keep the misleading label.l1_lambdaoutsideprox,proxwith L0/budget pruning,proxwithl2_lambda).Validation
uv run ruff format packages/populace-calibrate/src/populace/calibrate/solve.py packages/populace-calibrate/src/populace/calibrate/__init__.py packages/populace-calibrate/tests/test_solve.py --checkuv run ruff check packages/populace-calibrate/src/populace/calibrate/solve.py packages/populace-calibrate/src/populace/calibrate/__init__.py packages/populace-calibrate/tests/test_solve.pygit diff --checkuv run --project packages/populace-calibrate --group dev python -m pytest packages/populace-calibrate/tests/test_solve.py -q-> 42 passeduv run --project packages/populace-calibrate --group dev python -m pytest packages/populace-calibrate -q-> 106 passed, with existing torch sparse warningsReview notes
This supersedes the earlier old-base version of #184. Two review cycles were run. The first found prox projection wording and missing
l1_lambdadocs; both were fixed. The second found theapgrejection compatibility break, a prox all-zero error leak, and stale Adam-only public docs; all three were fixed here.