Rank earnings quintiles over adults on potential earnings - #1829
Conversation
Adversarial review — policyengine-uk PR 1829Verdict: REQUEST CHANGES.
Validation: 17 relevant tests passed across focused and broader sets, 11 credential-dependent tests skipped, Ruff passed. Both adversarial failures reproduce despite the passing tests. No check runs were attached to the head. |
Addresses review on #1829. Ranking a pooled distribution of workers' actual and non-workers' imputed earnings split the tied mass of non-workers across a quintile boundary, and which member of the mass fell on which side depended on the order of rows in the dataset. A permutation of the test fixture moved 23 of 80 adults, all of them non-workers. Elasticities and stochastic responses should not vary with serialisation. Thresholds are now the weighted quintile boundaries of the observed earnings distribution of working adults, and everyone is placed against them by value: workers on actual earnings, non-workers on imputed potential earnings. A tied mass lands wholly in the quintile its value belongs to, so the result is independent of row order — verified over permutations in the tests. This also drops the claim that the OBR note prescribes ranking workers and non-workers together, which it does not say. Thresholds estimated from observed earnings are a weaker and better-supported assumption. Quintiles are consequently not equal shares of the adult population: workers divide roughly 20% each, while non-workers concentrate where their imputed wage falls. That is intended — these are quintiles of the earnings distribution, which is what Table A1 is indexed on. weighted_median now falls back to the unweighted median for a group carrying no weight, rather than returning zero. A zero there silently bars entry into employment, and the previous `if donors.any()` guard did not catch it: a non-empty but weightless donor group returned zero and bypassed the fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
|
All three reproduced. Pushed fixes for each; the first two changed the approach rather than patching it. 1. Tie-order dependence — confirmed and fixedReproduced directly: permuting the fixture moved 23 of 80 adult quintiles, all of them non-workers tied on a shared imputed value. Fixed by taking your suggestion in point 2 rather than by adding a tiebreak, since that resolves both issues at once. Verified over 20 permutations: 0 changes, and there is now a test asserting it. 2. The pooled ranking is not supported by the OBR note — acceptedYou are right, and I should not have leaned on the note for that. I had already dropped the claim about what the OBR intended from #1827 for the same reason, but the implementation still embodied it. Thresholds are now the weighted quintile boundaries of the observed earnings distribution of working adults, with everyone placed against them by value — workers on actual earnings, non-workers on imputed potential earnings. That is the weaker, better-supported assumption you proposed, and it is what makes the result order-independent: a tied mass lands wholly in the quintile its value belongs to. The visible consequence, which I have documented in the docstring: quintiles are no longer equal shares of the adult population. On 3. Zero-weight donors — confirmed and fixedYour reproducer was right and my Where things stand on real data
13 tests in the two new files, 19 passed across the broader labour supply set, |
juaristi22
left a comment
There was a problem hiding this comment.
Adversarial re-review — request changes
The recent changes resolve the two main issues from my first review: fixed thresholds remove the row-order/tie dependence, and deriving those thresholds from observed worker earnings is the right distributional construction.
BLOCKING — zero-weight records still affect modeled outcomes (participation.py:278-293, 385-400).
weighted_median now falls back to the unweighted median when every donor has zero weight. That prevents impute_wages_for_nonworkers from reaching its documented overall-median fallback, so observations representing no population can dictate the imputed wage. In a minimal case, a zero-weight same-cell donor earning £1,000,000 and a positive-weight overall donor earning £40,000 produce £501,333 of imputed annual earnings for the nonworker.
weighted_quantiles has the same root problem: zero-weight observations remain in the interpolation array. Positive-weight values [10, 20, 30, 40, 50] give thresholds [15, 25, 35, 45]; inserting a zero-weight value of 29 changes them to [15, 29, 35, 45]. A zero-population row must not move a population-weighted boundary.
Please filter to finite, strictly positive weights for both donor medians and worker thresholds. If a demographic cell has no positive-weight donor, fall back to the overall positive-weight worker median. Add invariance tests showing that adding, removing, or changing zero-weight rows cannot affect imputation or quintiles.
The integration-style regression discussed on #1828 that executes the new-entrant hours_worked writeback is also not yet present at this head; per the thread there, #1829 can carry it.
Validation: 19 focused tests passed, 11 credential-dependent tests skipped; Ruff, formatting, and git diff --check passed. The two adversarial weight-invariance cases above exposed the remaining issue.
|
One further thing found while writing the integration test you asked for, which I have not fixed here and want on the record before this merges. The entry path crashes. It reproduces on a small situation-based simulation as soon as anyone actually enters work. There are at least two sites — This is latent on I stopped short of fixing it because it is a distinct defect from the two this PR addresses, and bundling a dtype fix into a change about quintiles and donor pools would make both harder to review. Happy to either add it here or file it separately — say which you prefer. It does mean the integration test you asked for on #1828 cannot be written until this is fixed, since forcing entry is exactly what triggers the crash. That is the honest reason it is not in this PR yet. |
Addresses review on #1829. Ranking a pooled distribution of workers' actual and non-workers' imputed earnings split the tied mass of non-workers across a quintile boundary, and which member of the mass fell on which side depended on the order of rows in the dataset. A permutation of the test fixture moved 23 of 80 adults, all of them non-workers. Elasticities and stochastic responses should not vary with serialisation. Thresholds are now the weighted quintile boundaries of the observed earnings distribution of working adults, and everyone is placed against them by value: workers on actual earnings, non-workers on imputed potential earnings. A tied mass lands wholly in the quintile its value belongs to, so the result is independent of row order — verified over permutations in the tests. This also drops the claim that the OBR note prescribes ranking workers and non-workers together, which it does not say. Thresholds estimated from observed earnings are a weaker and better-supported assumption. Quintiles are consequently not equal shares of the adult population: workers divide roughly 20% each, while non-workers concentrate where their imputed wage falls. That is intended — these are quintiles of the earnings distribution, which is what Table A1 is indexed on. weighted_median now falls back to the unweighted median for a group carrying no weight, rather than returning zero. A zero there silently bars entry into employment, and the previous `if donors.any()` guard did not catch it: a non-empty but weightless donor group returned zero and bypassed the fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
e80d3cf to
5f05bee
Compare
juaristi22
left a comment
There was a problem hiding this comment.
Program Review
Source Documents
- Official note: OBR, NICs cut impact on labour supply (Table A1)
- Scope: Full merge-base review, carrying forward prior findings because the earlier report did not record a reviewed SHA
- Reviewed head SHA:
5f05bee62c702fe1d5f56d87eae4d67352af1088 - Mode: full
Critical (Must Fix)
- [C3] Towncrier still ignores the PR's changelog fragment.
changelog.d/1827.mdhas no configured type;towncrier build --draftrenders “No significant changes.” Rename it with a valid type suffix or place it in a configured type directory. —changelog.d/1827.md:1
Should Address
- [A2] Adults in a population with no workers are assigned to quintile 5. With no worker observations,
weighted_quantilesreturns four zero thresholds andsearchsorted(..., side="right")assigns every non-working adult to the top quintile. A focused reproducer returned[5, 5]; add an explicit fallback and boundary test. —policyengine_uk/dynamics/participation.py:458
Resolved Prior Findings
- [C1 RESOLVED] Fixed worker thresholds remove row-order-dependent splitting of tied imputed earnings; five shuffle tests pass.
- [C2 RESOLVED] Workers define the earnings distribution; non-workers are placed against fixed thresholds rather than pooled into the ranking.
- [A1 RESOLVED] Zero-weight donor groups now fall back to an unweighted median and the overall donor fallback.
Validation Summary
- Focused tests: 9 passed; row-order reproducer fixed.
- New no-worker reproducer:
[5, 5], confirming A2. - Ruff: passed on changed Python files.
- CI: 8/8 checks passed on the exact head.
Review Severity: REQUEST_CHANGES
Next Steps
Fix the Towncrier path and define/test the no-worker fallback.
|
Both fixed. A2 — a population with no workers put every adult in the top quintile. Reproduced. With no observed earnings there is no distribution: My first attempt at a fallback did not work, and the reason is worth recording: I fell back to thresholds from potential earnings, but with no workers there are no wage donors either, so imputed wages are zero and the thresholds stay degenerate. The fix now keys on the degeneracy itself — if the thresholds carry no dispersion, adults are spread evenly by weighted rank rather than asserting a quintile. Zero total weight falls back to the middle quintile. Real data is unaffected, since workers exist and the thresholds have dispersion: quintile shares of workers remain 19.8 / 20.1 / 20.0 / 20.1 / 20.0. C3 — changelog. Renamed to Thanks for confirming C1, C2 and A1 as resolved, and for the no-worker reproducer — it is the kind of degenerate input I would not have constructed. Still outstanding on this PR and not fixed here: |
Addresses review on #1829. Ranking a pooled distribution of workers' actual and non-workers' imputed earnings split the tied mass of non-workers across a quintile boundary, and which member of the mass fell on which side depended on the order of rows in the dataset. A permutation of the test fixture moved 23 of 80 adults, all of them non-workers. Elasticities and stochastic responses should not vary with serialisation. Thresholds are now the weighted quintile boundaries of the observed earnings distribution of working adults, and everyone is placed against them by value: workers on actual earnings, non-workers on imputed potential earnings. A tied mass lands wholly in the quintile its value belongs to, so the result is independent of row order — verified over permutations in the tests. This also drops the claim that the OBR note prescribes ranking workers and non-workers together, which it does not say. Thresholds estimated from observed earnings are a weaker and better-supported assumption. Quintiles are consequently not equal shares of the adult population: workers divide roughly 20% each, while non-workers concentrate where their imputed wage falls. That is intended — these are quintiles of the earnings distribution, which is what Table A1 is indexed on. weighted_median now falls back to the unweighted median for a group carrying no weight, rather than returning zero. A zero there silently bars entry into employment, and the previous `if donors.any()` guard did not catch it: a non-empty but weightless donor group returned zero and bypassed the fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
5f05bee to
beec09e
Compare
Addresses review on #1829. Ranking a pooled distribution of workers' actual and non-workers' imputed earnings split the tied mass of non-workers across a quintile boundary, and which member of the mass fell on which side depended on the order of rows in the dataset. A permutation of the test fixture moved 23 of 80 adults, all of them non-workers. Elasticities and stochastic responses should not vary with serialisation. Thresholds are now the weighted quintile boundaries of the observed earnings distribution of working adults, and everyone is placed against them by value: workers on actual earnings, non-workers on imputed potential earnings. A tied mass lands wholly in the quintile its value belongs to, so the result is independent of row order — verified over permutations in the tests. This also drops the claim that the OBR note prescribes ranking workers and non-workers together, which it does not say. Thresholds estimated from observed earnings are a weaker and better-supported assumption. Quintiles are consequently not equal shares of the adult population: workers divide roughly 20% each, while non-workers concentrate where their imputed wage falls. That is intended — these are quintiles of the earnings distribution, which is what Table A1 is indexed on. weighted_median now falls back to the unweighted median for a group carrying no weight, rather than returning zero. A zero there silently bars entry into employment, and the previous `if donors.any()` guard did not catch it: a non-empty but weightless donor group returned zero and bypassed the fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
|
A second adversarial pass found three more, two of them serious. All fixed, and two of my own claims were wrong. The dtype crash is unconditional, not entry-conditional — and I had the wrong site. It fires on the first My explanation of why this is latent on Non-worker quintiles carried almost no information, and it was a unit mismatch. Imputed earnings assume 18.8 hours a week while the thresholds come from observed earnings at whatever hours workers actually do — mostly full time. That pushed every non-worker down by roughly the hours ratio: all 12 distinct imputed values fell inside a single threshold interval, so 98.3% landed in Q2 and Q3–Q5 were unreachable for any reform on any dataset. Non-workers are now placed on a full-time-equivalent basis, with the entrant's actual earnings left at the assumed hours. On the real data at 2027:
Workers stay at 19.8 / 20.1 / 20.0 / 20.1 / 20.0. Three of five rather than five is still not ideal, and the cause is the coarse donor grouping — only 12 distinct imputed values, one per sex and age band. That is your finding 5 and I have not changed the banding here.
Also fixed from the earlier pass: the no-worker case (my first attempt at that fallback did not work, because with no workers there are no donors either, so potential earnings were degenerate too) and the changelog type suffix. Not addressed, and I think correctly out of scope here: the coarse age bands, You are right that the green suite was not evidence the PR worked end to end — every test was |
beec09e to
9218c11
Compare
calculate_earnings_quintile applied pd.qcut to raw employment_income across
every person in the dataset, children included. More than half that
population has no earnings, so the bottom two quintiles were entirely
non-earners and roughly a quarter of each lower quintile were children.
Two consequences. The OBR Table A1 elasticities are indexed by this quintile
and rise steeply as it falls — lone_parent_3_5 runs from 1.554 at Q1 to
0.155 at Q5 — so every potential entrant sat at the steep end. And because
impute_wages_for_nonworkers drew donors from the same elasticity groups, the
lowest quintiles had no employed donors, so their non-workers were imputed a
wage of zero. apply_participation_responses gates entry on
imputed_wages[i] > 0, so those people could never enter employment under any
reform — silently, with no error.
The function's docstring already described the intended behaviour ("For
workers, uses actual earnings. For non-workers, uses imputed potential
earnings"); it was not implemented, and could not be while the imputation
depended on the quintile.
Wage donors are now grouped by sex and age band, which breaks that circular
dependency, and quintiles are ranked over adults on potential earnings.
Assignment is by weighted rank rather than value cutoffs because every
non-worker in a donor group shares one imputed value, and cutting on values
drops a whole tied mass into a single quintile.
On enhanced_frs_2024_25 at 2027: quintiles are 20.0% of adults each, no
non-working adult is left with a zero imputed wage (previously 100% of Q1),
and imputed entrant earnings average £18,004, an implied £18.42 an hour.
Fixes #1827
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
Addresses review on #1829. Ranking a pooled distribution of workers' actual and non-workers' imputed earnings split the tied mass of non-workers across a quintile boundary, and which member of the mass fell on which side depended on the order of rows in the dataset. A permutation of the test fixture moved 23 of 80 adults, all of them non-workers. Elasticities and stochastic responses should not vary with serialisation. Thresholds are now the weighted quintile boundaries of the observed earnings distribution of working adults, and everyone is placed against them by value: workers on actual earnings, non-workers on imputed potential earnings. A tied mass lands wholly in the quintile its value belongs to, so the result is independent of row order — verified over permutations in the tests. This also drops the claim that the OBR note prescribes ranking workers and non-workers together, which it does not say. Thresholds estimated from observed earnings are a weaker and better-supported assumption. Quintiles are consequently not equal shares of the adult population: workers divide roughly 20% each, while non-workers concentrate where their imputed wage falls. That is intended — these are quintiles of the earnings distribution, which is what Table A1 is indexed on. weighted_median now falls back to the unweighted median for a group carrying no weight, rather than returning zero. A zero there silently bars entry into employment, and the previous `if donors.any()` guard did not catch it: a non-empty but weightless donor group returned zero and bypassed the fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
9218c11 to
0b60e09
Compare
Addresses review on #1829. Ranking a pooled distribution of workers' actual and non-workers' imputed earnings split the tied mass of non-workers across a quintile boundary, and which member of the mass fell on which side depended on the order of rows in the dataset. A permutation of the test fixture moved 23 of 80 adults, all of them non-workers. Elasticities and stochastic responses should not vary with serialisation. Thresholds are now the weighted quintile boundaries of the observed earnings distribution of working adults, and everyone is placed against them by value: workers on actual earnings, non-workers on imputed potential earnings. A tied mass lands wholly in the quintile its value belongs to, so the result is independent of row order — verified over permutations in the tests. This also drops the claim that the OBR note prescribes ranking workers and non-workers together, which it does not say. Thresholds estimated from observed earnings are a weaker and better-supported assumption. Quintiles are consequently not equal shares of the adult population: workers divide roughly 20% each, while non-workers concentrate where their imputed wage falls. That is intended — these are quintiles of the earnings distribution, which is what Table A1 is indexed on. weighted_median now falls back to the unweighted median for a group carrying no weight, rather than returning zero. A zero there silently bars entry into employment, and the previous `if donors.any()` guard did not catch it: a non-empty but weightless donor group returned zero and bypassed the fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
0b60e09 to
b6f2088
Compare
juaristi22
left a comment
There was a problem hiding this comment.
Program Review
Source Documents
- Official note: OBR, The labour supply effects of the Autumn 2023 National Insurance Contributions cut, especially paragraph 19 and Table A1.
- Underlying methodology: Adam and Phillips (2013), Appendix E, especially pages 133–136.
- Reviewed head SHA:
b6f20884fda600ca0fb13cb4f98cb4a4e49244ee - Mode: full
- Scope: Full merge-base review. The previously reviewed head was not an ancestor of this force-pushed head.
Critical (Must Fix)
- [C1] The new degenerate-distribution fallback is still row-order dependent. When all four thresholds are equal, lines 510–517 sort tied potential earnings stably and assign quintiles by cumulative row rank. For ten identical non-working adults, the function returned
[1,1,2,2,3,3,4,4,5,5]; after shuffling the input and restoring the original identities it returned[4,2,2,5,1,1,3,4,5,3]. Thus identical records receive different elasticities solely because of serialization order, contradicting the function's documented invariant. This path covers all-non-worker and zero-dispersion simulations. Preserve ties as a block or use a deterministic characteristic-based fallback, and add a permutation test that enters this branch. —policyengine_uk/dynamics/participation.py:503
Should Address
- [A1] Full-time-equivalent scaling is a material, unsupported extrapolation from the cited method. The OBR says entrants are assumed to work 18.8 hours weekly and labels Table A1 by position in the UK earnings distribution; it does not say to multiply their imputed in-work income to 37.5-hour FTE for elasticity assignment. The underlying IFS method is even less direct: it applies the calibration to existing workers and says non-workers are not allocated elasticities, with their potential earnings only implicit in the group judgement. On the focused fixture, the new factor moves every entrant from Q1 at £25,838 imputed part-time income to Q3, cutting the assigned Table A1 elasticity by about 40% for each demographic row. The latest code now acknowledges that real-data entrants still occupy only Q3/Q4, so the table remains near-degenerate; this makes a calibration or sensitivity result more important, not less. Either support 37.5/FTE placement with evidence or retain the 18.8-hour potential income and document a sensitivity range. —
policyengine_uk/dynamics/participation.py:477
Resolved Prior Findings
- [C2 RESOLVED] Worker-derived thresholds avoid splitting tied non-worker values in the ordinary, non-degenerate path.
- [A2 RESOLVED] A no-worker population is no longer forced wholly into quintile 5, although its replacement fallback introduces C1.
- [C3 RESOLVED]
changelog.d/1827.fixed.mdis now consumed by Towncrier. - [A3 RESOLVED] Zero/non-finite total weights fall back to unweighted quantiles; pandas 3 dtype assignments are handled explicitly.
- [A4 RESOLVED] The latest commit filters non-finite value/weight pairs and rejects mismatched shapes in
weighted_median.
Validation Summary
- Focused tests:
13 passed. - Adversarial permutation reproducer: failed invariance on the exact head (
invariant False). - FTE sensitivity fixture: non-workers move from Q1 to Q3.
- Towncrier: fragment renders under Fixed.
- Exact-head GitHub CI at final check: 7 passed, Test in progress; GitHub reports the PR mergeable.
Review Severity: REQUEST_CHANGES
Next Steps
Make the equal-threshold path tie- and order-invariant, then justify or sensitivity-test the FTE placement assumption.
Addresses review on #1829. Ranking a pooled distribution of workers' actual and non-workers' imputed earnings split the tied mass of non-workers across a quintile boundary, and which member of the mass fell on which side depended on the order of rows in the dataset. A permutation of the test fixture moved 23 of 80 adults, all of them non-workers. Elasticities and stochastic responses should not vary with serialisation. Thresholds are now the weighted quintile boundaries of the observed earnings distribution of working adults, and everyone is placed against them by value: workers on actual earnings, non-workers on imputed potential earnings. A tied mass lands wholly in the quintile its value belongs to, so the result is independent of row order — verified over permutations in the tests. This also drops the claim that the OBR note prescribes ranking workers and non-workers together, which it does not say. Thresholds estimated from observed earnings are a weaker and better-supported assumption. Quintiles are consequently not equal shares of the adult population: workers divide roughly 20% each, while non-workers concentrate where their imputed wage falls. That is intended — these are quintiles of the earnings distribution, which is what Table A1 is indexed on. weighted_median now falls back to the unweighted median for a group carrying no weight, rather than returning zero. A zero there silently bars entry into employment, and the previous `if donors.any()` guard did not catch it: a non-empty but weightless donor group returned zero and bypassed the fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
b6f2088 to
5ede79a
Compare
|
@juaristi22 C1 confirmed and fixed; A1 documented rather than changed, with the bound quantified. C1 — the degenerate fallback was still order-dependent. Reproduced exactly as you describe: ten identical non-working adults returned The fix blocks on the value rather than the row: each distinct potential-earnings value is placed at the midpoint of its own block of weight, so identical records are placed identically by construction. Ten identical adults now all get quintile 3 — the middle of the table, which is the honest answer when there is no dispersion to read — while genuinely distinct values are still spread rather than collapsed. Two tests added, both driving the real A1 — FTE placement. I agree this is an extrapolation the sources do not make, and I am not going to dress it up as one they do. Rather than assert it, the module now records it as an assumption with the direction and size of the bound: unscaled 18.8-hour placement moves entrants to quintile 1 and roughly doubles the elasticity each demographic row draws, and callers wanting that bound can pass One correction to my own earlier comment while re-running on |
|
@juaristi22 re-review requested. Since your 13:32 pass: C1's tie-ordering is fixed by blocking on the value rather than the row (ten identical adults now all get quintile 3), with two tests driving the real function through the equal-threshold branch — both fail on the old code with your |
juaristi22
left a comment
There was a problem hiding this comment.
Program Review
Source Documents
- Official note: OBR, The labour supply effects of the Autumn 2023 National Insurance Contributions cut, especially paragraph 19 and Table A1.
- Underlying methodology: Adam and Phillips (2013), Appendix E, especially pages 133–136.
- Reviewed head SHA:
5ede79acb41b590ba8afb19a8353e8ca67e0b6ba - Mode: full patch comparison after rebase; incremental semantic comparison from reviewed head
b6f20884fda600ca0fb13cb4f98cb4a4e49244ee.
Critical (Must Fix)
- [C1 STILL OPEN] The ordinary weighted-threshold path remains row-order dependent when equal-earning workers carry unequal weights. The latest commit fixes the all-equal fallback by aggregating tied potential values, but
weighted_quantilesstill sorts rows and interpolates from each row's weight midpoint without first aggregating equal values. Swapping two £10,000 workers with weights 1 and 2 changed the thresholds from[10000, 11600, 16400, 23000]to[10000, 10000, 16400, 23000]; after restoring identities, the exact-head function changed the two identical workers from Q2 to Q3. Their elasticities therefore still depend on serialization order. Aggregate weights by distinct earnings value before computing the midpoint CDF, and add a permutation test with tied earnings and unequal weights. —policyengine_uk/dynamics/participation.py:411
Should Address
- [A1 ACKNOWLEDGED] Full-time-equivalent placement remains a material unsupported modeling assumption. The code and changelog now state clearly that the sources do not prescribe it, quantify the approximate twofold elasticity bound, and point to #1836. That transparency is sufficient for this PR, but the assumption still needs calibration or sensitivity validation before treating the participation result as settled. —
policyengine_uk/dynamics/participation.py:477 - [A5 NEW] The Towncrier fragment renders with a duplicated bullet. The file starts with
-, while the template adds its own list marker, producing- - Place earnings quintiles.... Remove the leading dash. —changelog.d/1827.fixed.md:1
Resolved Prior Findings
- [C1 PARTIALLY RESOLVED] The exact all-equal fallback reproducer is now tie- and order-invariant; the same invariant still fails in the ordinary weighted-threshold path described above.
- [C2 RESOLVED] Worker-derived thresholds avoid splitting tied non-worker masses by rank.
- [A2 RESOLVED] A no-worker population is not forced wholly into quintile 5.
- [C3 RESOLVED] The changelog fragment is consumed by Towncrier.
- [A3 RESOLVED] Zero/non-finite weights and pandas 3 dtype assignments are handled.
- [A4 RESOLVED]
weighted_medianfilters non-finite pairs and rejects mismatched shapes.
Validation Summary
- Focused tests:
15 passed. - Adversarial tied-income/unequal-weight permutation reproducer: failed on the exact head (
[2,2,3,4,5]versus restored[3,3,3,4,5]). - Ruff: passed on changed Python files.
- Towncrier: fragment is consumed but renders a duplicated bullet.
- Exact-head GitHub CI: all 8 checks passed; PR is mergeable.
Review Severity: REQUEST_CHANGES
Next Steps
Aggregate equal worker earnings into weighted blocks before interpolating thresholds, then add the unequal-weight permutation regression test.
Addresses review on #1829. Ranking a pooled distribution of workers' actual and non-workers' imputed earnings split the tied mass of non-workers across a quintile boundary, and which member of the mass fell on which side depended on the order of rows in the dataset. A permutation of the test fixture moved 23 of 80 adults, all of them non-workers. Elasticities and stochastic responses should not vary with serialisation. Thresholds are now the weighted quintile boundaries of the observed earnings distribution of working adults, and everyone is placed against them by value: workers on actual earnings, non-workers on imputed potential earnings. A tied mass lands wholly in the quintile its value belongs to, so the result is independent of row order — verified over permutations in the tests. This also drops the claim that the OBR note prescribes ranking workers and non-workers together, which it does not say. Thresholds estimated from observed earnings are a weaker and better-supported assumption. Quintiles are consequently not equal shares of the adult population: workers divide roughly 20% each, while non-workers concentrate where their imputed wage falls. That is intended — these are quintiles of the earnings distribution, which is what Table A1 is indexed on. weighted_median now falls back to the unweighted median for a group carrying no weight, rather than returning zero. A zero there silently bars entry into employment, and the previous `if donors.any()` guard did not catch it: a non-empty but weightless donor group returned zero and bypassed the fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
5ede79a to
9de879b
Compare
|
@juaristi22 C1 confirmed again — you were right that I fixed the fallback and not the ordinary path. Same bug class, different function.
Two tests, both failing on the previous head:
I searched for that second fixture rather than assert one: my first attempt passed on the old code, so it would have been a test that never failed. A5 fixed — the fragment started with On real data |
juaristi22
left a comment
There was a problem hiding this comment.
Program Review
Source Documents
- Official note: OBR, The labour supply effects of the Autumn 2023 National Insurance Contributions cut, especially paragraph 19 and Table A1.
- Underlying methodology: Adam and Phillips (2013), Appendix E, especially pages 133–136.
- Reviewed head SHA:
9de879b9aca1d9056abd60afa442e95be83824a9 - Mode: full patch comparison after force-push; incremental semantic comparison from
5ede79acb41b590ba8afb19a8353e8ca67e0b6ba.
Critical (Must Fix)
- None.
Should Address
- [A1 STILL OPEN, ACKNOWLEDGED] Full-time-equivalent placement remains a material unsupported modeling assumption. The implementation now documents that the cited sources do not prescribe the 37.5-hour scaling, quantifies that unscaled 18.8-hour placement roughly doubles entrant elasticities, and tracks validation in #1836. This is transparent enough not to block this corrective PR, but downstream participation results should retain that sensitivity caveat. —
policyengine_uk/dynamics/participation.py:484
Resolved Prior Findings
- [C1 RESOLVED]
weighted_quantilesnow aggregates weights by distinct earnings value before constructing the CDF. The exact prior £10k tied-worker/unequal-weight permutation reproducer now returns identical restored quintiles ([2,2,3,4,5]both ways), and two focused regression tests cover thresholds and end-to-end placement. —policyengine_uk/dynamics/participation.py:411 - [A5 RESOLVED] The leading dash was removed from the changelog fragment; Towncrier now renders a single bullet.
- Earlier worker-threshold, degenerate-distribution, zero-weight, finite-value, dtype, and entity/test findings remain resolved.
Validation Summary
- Focused tests:
17 passed. - Exact adversarial permutation reproducer: passed.
- Ruff: passed on changed Python files.
- Towncrier: Fixed fragment renders correctly.
- Exact-head GitHub CI: all 8 checks passed; PR is mergeable.
Review Severity: APPROVE_WITH_COMMENT
Next Steps
The row-order bug is fixed. Keep the FTE sensitivity visible until #1836 supplies an empirical calibration.
Fixes #1827.
The bug
calculate_earnings_quintileappliespd.qcutto rawemployment_incomeacross every person in the dataset, children included:More than half that population has zero earnings. On
enhanced_frs_2024_25(N = 113,617; 50.7% zero-earners; 20.5% under 18):Two consequences
Elasticities are skewed.
calculate_participation_elasticitiesindexes OBR Table A1 by this quintile, and the table rises steeply as the quintile falls —lone_parent_3_5runs[1.554, 1.243, 0.932, 0.621, 0.155], a 10× spread. Every potential entrant sat at the steep end.Most non-workers could never enter employment.
impute_wages_for_nonworkersdrew donors from the same elasticity groups, requiring(elasticities == v) & working_maskto be non-empty. The lowest quintiles had no workers, so imputation silently returned zero:and
apply_participation_responsesgates entry on it:So roughly 24,000 of 43,000 non-working adults were structurally incapable of entering work under any reform. No error, just no entrants.
Against the existing docstring
That is what the function was meant to do. It could not, while the imputation depended on the quintile — the dependency was circular.
hours_for_new_entrantswas accepted and never used.Changes
random_seedis retained in the signature for compatibility and documented as unused.Effect on
enhanced_frs_2024_25at 2027Not fixed here
I have not asserted what quintile definition the OBR intended — I could not extract text from the OBR note. The case rests on the implementation's own terms: two quintiles that are entirely non-earners, contain no observed wage, and block entry for everyone in them cannot be a sound input to a table indexed by earnings rank.
The ranking is now weighted; it was previously unweighted over the microdata array.
Testing
Plus the 4 from #1828. Existing
test_labour_supply_response_formulas.pyandtest_behavioral_responses.pystill pass (17 passed, 11 skipped without a dataset token).ruff formatclean.