Add tax_free_childcare_spend_routed_share as a neutral-default input - #1834
Conversation
964dcfc to
7527c50
Compare
|
Rebased onto |
juaristi22
left a comment
There was a problem hiding this comment.
Program Review
Source Documents
- Act PDF: Childcare Payments Act 2014 (c. 28) (54 pages; all rendered at 300 DPI; disputed pages 7, 14, and 18 re-rendered at 600 DPI)
- HMRC manual: TFC35300 — calculating the top-up element and maximum
- Statistics: Tax-Free Childcare statistics, March 2026 and quality report
- Year: 2025 / 2024-25 aggregate comparison
- Scope: PR changes only; interaction with PR 1830, prior PR 1833, and companion policyengine-uk-data PR 473 checked
- Reviewed head SHA: 7527c50 (GitHub base
mainat19e67fe6; the PR body still incorrectly describes a stack on #1830) - Mode: full
Critical (Must Fix)
-
[C1] The new tests and target rationale still treat gross provider spend as a parent deposit.
childcare_expensesis a Person input documented as the total amount spent on childcare, and thecontribution.rateparameter is 0.2 of total household-plus-government contributions. Buttax_free_childcare.py:40-42computesgross * 0.2 / 0.8 = 25% of gross; the new tests attax_free_childcare.yaml:119-138therefore expect £500 on £2,000 of total childcare spend. The Act makes the top-up 25% of the qualifying payment into the account (file p.7); section 21 converts that to 20% of the topped-up total (file p.18). Under the repository's input contract, £2,000 gross spend supports £400, not £500. The formula pre-exists on main, but PR 1834 newly codifies it and its reported 1.25x target result depends on it. Resolve the gross/net contract first and re-score; companion PR 473 itself says the gross interpretation would move the result to about 1.00x target. -
[C2] HMRC's active-month ratio is not an observed routed-spend share, and moving the same constant to data does not preserve spread. The new input is described as the share of annual childcare spending routed through TFC (
tax_free_childcare_spend_routed_share.py:7-17). HMRC instead defines a used account as one with at least one payment from the account to a provider in the period. The verified 2024-25 arithmetic, 7,715,605 monthly child-account observations / 1,085,020 annual unique children / 12 = 0.592585, measures the average fraction of months with any provider payment. It is not value-weighted, does not measure deposits that attract top-up, and HMRC notes payment timing need not match care timing and payments can occur after eligibility ends. Moreover, PR 1834 says per-benefit-unit assignment preserves a distributional spread, while companion PR 473 explicitly assigns the same constant to every BenUnit because no distribution is observed. The 1.25x result is therefore a sensitivity estimate under an unproven uniform-monthly-spend assumption, not source validation. Keep the neutral model input if useful, but name/document the empirical value as a proxy or calibration adjustment and state the assumption; do not call 0.593 an observed routed-spend share.
Should Address
-
[A1] The input is attached to the wrong granularity for the legal/source unit. Each TFC account is held for one child only (Act section 15(2), file p.14); Table 2 is child-level;
childcare_expensesandtax_free_childcareare Person-level. A BenUnit-wide share cannot represent siblings with different account use and can change totals when per-child caps bind. Make the input Person/child-level or document and test a defensible aggregation rule. —tax_free_childcare_spend_routed_share.py:4-6,tax_free_childcare.py:37 -
[A2] Complete the input contract. This dimensionless share has no
unit = "/1"(runtime metadata reportsNone), and neither metadata nor formula establishes the valid 0-1 range. Add the unit and an explicit range/validation convention so negative or above-one data cannot create negative or supra-statutory awards. Describe default 1 as a neutral all-spend-routed assumption, not a statutory requirement. —tax_free_childcare_spend_routed_share.py:4-17 -
[A3] Add the boundaries and household shape that would catch the modeling risks. The three added cases cover default 1, 0.5, and a cap, but omit share 0, invalid bounds, a multi-child asymmetric-routing case, and the combined part-year interaction previously present on the stack. Also guard against future data populating both
eligible_declaration_periodsand this share from the same activity-duration series, which would double-scale spend. —tax_free_childcare.yaml:115-150 -
[A4] Rebase and review the real #1830 combination. The current GitHub head targets main and excludes #1830, despite the PR body claiming it is stacked. Both PRs edit the contribution path, and this head does not apply #1830's
eligible_fractionto contribution. Resolve the overlap deliberately and rerun household, statutory, and aggregate validation after #1830 lands.
Suggestions
- [S1] Make the merge dependency explicit. PR 1834 alone deliberately leaves microsimulation unchanged and the TFC aggregate high; PR 473 cannot safely ship first because older model versions do not declare the input. Coordinate model-then-data release order and remove the PR-body claim that the companion preserves spread unless the companion actually becomes heterogeneous.
PDF Audit Summary
| Category | Count |
|---|---|
| Confirmed correct | 2 |
| Mismatches (code-path confirmed + visually verified) | 1 |
| Mismatches rejected (code-path cleared) | 0 |
| Unmodeled items | 1 |
| Pre-existing issues | 1 |
The confirmed mismatch is the gross-spend contribution base. The unmodeled item is child-level routing/account use. The formula predates this one-commit diff on main, but the new tests and aggregate claim depend on it.
Validation Summary
| Check | Result |
|---|---|
| Regulatory Accuracy | 1 confirmed formula mismatch; 1 unsupported empirical interpretation |
| Reference Quality | Broad link does not corroborate routed-spend share; exact official sources identified |
| Code Patterns | 4 contract/entity/stack issues |
| Formatting | Ruff check and format passed; git diff --check passed |
| Test Coverage | boundary/entity/combined-stack gaps; focused 14/14 passed |
| Broader Regression | HMRC policy suite 372/372 passed |
| PDF Value Audit | 1 mismatch / 2 confirmed statutory points |
| CI Status | 16 checks attached to head 7527c50; lint passed while remaining jobs were queued/in progress at capture; frozen-lock local tests pass |
Review Severity: REQUEST_CHANGES
Next Steps
Resolve the gross/net contribution base before interpreting the aggregate target; recast 0.593 as a documented duration proxy rather than an observed spend share; align the input with the child/account entity; then actually rebase/re-score the PR 1830 + 1834 + 473 combination and add the missing boundary/multi-child/combined-stack tests.
7527c50 to
1d92052
Compare
|
All four points addressed, and two of them caught real contradictions between my own PRs. C1 — gross/net. Accepted and fixed in #1830, using the sources you found. The clincher was in the repo itself: Your prediction was right on the numbers. With both changes on
C2 — the ratio is not an observed routed-spend share. Accepted. It measures months with any provider payment, is not value-weighted, and using it as a spending share assumes uniform spend across active months. The PR body now says that explicitly and calls it a proxy and calibration adjustment. You also caught a straight contradiction: #1834 claimed per-benefit-unit assignment preserves spread while #473 assigns a constant. I wrote both; the claim is removed. A1 — entity. Accepted, now A2 — contract. Added A3 — coverage. Added zero share, both clipping bounds, and the sibling case. The combined part-year interaction is covered on #1830 rather than duplicated here. Your double-scaling warning is a fair one: if a future data change populated both A4 — stack. The body was stale; I rebased onto S1 — release order. Agreed, and noted on #473: the model change has to ship first, since older versions do not declare the input. |
|
Merge-order note: this and #1830 are each mergeable against Suggested order is #1830 then #1834: #1830 is the statutory correctness fix and changes results on its own, #1834 adds an input that defaults to neutral and changes nothing until the companion data PR lands. I have verified the combined result locally. On
|
juaristi22
left a comment
There was a problem hiding this comment.
Program Review
Source Documents
- Law: Childcare Payments Act 2014, section 15
- Contribution rule: Childcare Payments Act 2014, sections 1 and 21
- Reviewed head SHA:
1d92052b40cf1bee971bb69f70877472b7357926 - Mode: full
Critical (Must Fix)
- [C1 STILL OPEN] The standalone head still applies 25% to gross childcare spend. The formula divides
p.rateby1 - p.rate, and the new tests still expect £500 on £2,000 gross spend. That conflicts with #1830's corrected 20%-of-gross implementation and the Act/input contract. The exact #1830/#1834 heads also produce content conflicts in the formula and YAML tests, so the claimed stack is not mergeable as reviewed. —policyengine_uk/variables/gov/hmrc/tax_free_childcare/tax_free_childcare.py:44 - [C3 NEW] The companion data PR writes this now-person variable on the benefit-unit table. #1834 correctly changes the entity to
Person, but data #473 still assignstax_free_childcare_spend_routed_sharetope_benunit. The published input therefore cannot populate the child-level variable contract without a coordinated change and exact-head integration build. —policyengine_uk/variables/gov/hmrc/tax_free_childcare/tax_free_childcare_spend_routed_share.py:5; data #473policyengine_uk_data/datasets/frs.py:1434
Should Address
- [A5 NEW] The rendered changelog text contradicts the updated model. It still calls the variable a family-level measured share and default 1 “the statutory position,” while the code now says child-level, neutral assumption, and the data PR says duration proxy. —
changelog.d/tfc-spend-routed-share.added.md:1
Resolved Prior Findings
- [A1 RESOLVED] Entity changed from BenUnit to Person and sibling-specific tests added.
- [A2 RESOLVED] Unit
/1, clipping, and zero/negative/above-one boundary tests added. - [A3 RESOLVED] Person-level formula use matches the declared entity.
- [C2 PARTLY RESOLVED] Documentation no longer claims HMRC measures a value-weighted routed-spend share, but the data-side proxy remains unsupported as an expenditure scaler.
Validation Summary
- Focused TFC tests: 18 passed, but they encode the still-wrong 25%-of-gross formula.
- Ruff: passed.
- Towncrier: renders the Added fragment, whose content is stale.
- CI: 8/8 checks passed on the exact head.
- Exact-head merge-tree with #1830: conflicts in formula and tests.
Review Severity: REQUEST_CHANGES
Next Steps
Rebase/stack on #1830, resolve the 20%-of-gross formula and expectations, update #473 to write the person-level field, and run a combined model/data build.
|
Correction to my earlier merge-order note: the conflict between this and #1830 is semantic, not textual. Resolving the overlapping hunks is not enough. #1834's routing tests encode the pre-#1830 rate — gross × 0.25 — including an explicit So whichever merges second has to re-derive those seven expectations, not just resolve the diff. Verified by merging both in a scratch copy; the merged aggregate is £0.6444bn at a routed share of 0.593, which is 1.02× the £0.632bn target. |
PolicyEngine/policyengine-uk#1834 makes tax_free_childcare_spend_routed_share a Person variable: a Tax-Free Childcare account is held for one child (Childcare Payments Act 2014 section 15(2)), and both childcare_expenses and tax_free_childcare are person-level, so a benefit-unit share cannot represent siblings with different account use. This build still wrote it to pe_benunit, so the published input would not have populated the variable it is meant to feed. Moved to pe_person, alongside the other person-level stochastic assignments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
1d92052 to
2dbbc41
Compare
|
All three addressed. C1 — restacked on #1830. You were right that leaving this on C3 — the companion data PR wrote the wrong entity. Good catch, and it was a break I introduced: I changed this variable to A5 — changelog. Rewritten: per-child rather than family-level, and default 1 described as a neutral all-spend-routed assumption rather than "the statutory position". On C2 remaining partly open — agreed, and I have not tried to close it. The data-side value is a duration proxy used as an expenditure scaler; #473 now says so in the parameter file and its title. What HMRC publishes is months with any provider payment, not a value-weighted routed share, and no source for the latter exists that I can find. One caveat on the calibration argument, which I have also noted on #1830: the combined result reaching about 1.02× of target is not evidence for the 20% rate specifically. The same target is reachable under the old 25% reading with a routed share of about 0.474. The case for 20% rests on the Act and on |
a345ecf to
25a588f
Compare
2dbbc41 to
6ebe644
Compare
juaristi22
left a comment
There was a problem hiding this comment.
Program Review
Source Documents
- Law: Childcare Payments Act 2014, section 15 and sections 1/21.
- Companion data implementation: policyengine-uk-data #473, head
fb8fc09193178581514d8d4fbc932af38c209136. - Reviewed head SHA:
6ebe6448efa4f085412c8d6e5df9cbbb54ff1aa3 - 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 denominator documentation is mathematically backwards and makes fractional eligibility double-discount the same months. Lines 18–22 define
routed_shareas the share of whole-year spending routed through the account. But the formula at lines 71–73 also multiplies byeligible_fraction. If a family is eligible for half the year and routes spending in three months, a whole-year share is 3/12 = 0.25; the formula returnsannual_spend * 0.5 * 0.25, only half of the routed amount. The correct factorizations are eitherannual_spend * whole_year_routed_shareorannual_spend * eligible_fraction * routed_share_conditional_on_eligible_period. Companion data #473 explicitly suppliesactive months / 12, a whole-year duration proxy, so the current contract selects the first denominator while the formula implements the second. Current built data masks this becauseeligible_declaration_periodsis binary, but the model exposes and tests fractional periods. Correct the denominator/formula contract and add a combined fractional-period × routed-share test. —policyengine_uk/variables/gov/hmrc/tax_free_childcare/tax_free_childcare_spend_routed_share.py:18;policyengine_uk/variables/gov/hmrc/tax_free_childcare/tax_free_childcare.py:71
Should Address
- [A1] Public documentation still makes the data value sound directly observed. Both the variable documentation and rendered changelog say the dataset sets the routed-spend share “from observed account use.” The companion #473 now correctly describes the HMRC statistic as a duration proxy: monthly active-account counts divided by annual unique children, applied as an expenditure scaler under uniform-spending/full-routing assumptions. Replace the sentence with wording such as “The dataset build may supply an HMRC-derived account-activity duration proxy; see policyengine-uk-data” so users do not read this as an observed routed-expenditure share. —
policyengine_uk/variables/gov/hmrc/tax_free_childcare/tax_free_childcare_spend_routed_share.py:16;changelog.d/tfc-spend-routed-share.added.md:1
Resolved Prior Findings
- [C2 RESOLVED] The branch includes #1830's corrected 20%-of-gross formula and eligible-spend prorating.
- [C3 RESOLVED] #1834 is based directly on the exact #1830 head; the previous formula/YAML conflict is gone.
- [C4 RESOLVED] Companion data #473 now writes the input to
pe_person, matching the child-levelPersonentity. - [A2 RESOLVED] Unit
/1, neutral default, clipping, zero/negative/above-one boundaries and sibling-specific behavior are covered. - [A3 RESOLVED] Section 15(2) supports the per-child account/entity choice.
Validation Summary
- Focused Tax-Free Childcare tests:
22 passedon the combined stack. - Towncrier: Added and inherited Fixed fragments render.
- Exact-head GitHub CI: not yet reported after the final force-push; GitHub reports the PR mergeable.
- Cross-repo contract: current #473 head writes the variable on the person table and explicitly frames its value as a proxy.
Review Severity: REQUEST_CHANGES
Next Steps
Choose a single routed-share denominator, align the formula and #473 proxy with it, and add a combined fractional-eligibility test; then align the proxy wording.
6ebe644 to
d257863
Compare
|
@juaristi22 C1 is right and my documentation had it exactly backwards. Fixed, along with A1. C1 — the denominator was inverted. I wrote that the share is measured across the whole year because the formula also multiplies by
A family eligible six months that routes spending in three of them has £6,000 of spend in the eligible window, £3,000 of it routed, so £600 of top-up. The formula implements the eligible-period denominator, so I have changed the contract to match the formula rather than the formula to match my prose. Two combined fractional-period × routed-share tests added. Cross-repo. #473 supplies months-paying / 12, a whole-year figure. That is not a conflict on any built data, because A1 — wording. Both the variable documentation and the changelog now say the dataset build may supply an HMRC-derived account-activity duration proxy rather than an observed routed-expenditure share, and point at policyengine-uk-data. 382 gov/hmrc policy tests pass on the combined stack. One note on your validation summary: your earlier run of the focused tests would have picked up whichever |
Tax-Free Childcare tops up money paid through the account, not a family's whole childcare bill, and childcare_expenses is annual. The model had no way to express the difference. Per child, not per benefit unit: a Tax-Free Childcare account is held for one child only (Childcare Payments Act 2014 section 15(2)), and both childcare_expenses and tax_free_childcare are person-level, so a benefit-unit share could not represent siblings with different account use and would shift totals where the per-child cap binds. Defaults to 1 — a neutral all-spend-routed assumption rather than a statutory requirement — so no household calculation and no built dataset changes. The value is clipped to 0-1 in the formula, since data outside that range would otherwise produce a negative or supra-statutory award. The empirical value deliberately lives in policyengine-uk-data. A model parameter applies to single-household calculations as well as microsimulation, so a population average would tell an individual family they receive a fraction of their statutory entitlement, which is wrong for them even where it is right on aggregate. That mirrors would_claim_tfc: the model declares the input, the data supplies the values. Tests cover the default, a half share, a capped award, a zero share, clipping above one and below zero, and siblings routing different shares. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NKBpgpwwY5ZXHUwh1GJQT
|
Rebased onto #1830 was squash-merged, so its commit was not an ancestor of
No duplicated YAML cases (24 distinct test names), and 382 gov/hmrc policy tests pass against merged |
d257863 to
a6d6056
Compare
|
@juaristi22 re-review requested. Since your 13:33 pass: C1 is fixed by correcting the denominator contract to the eligible period — you were right that my documentation had it backwards — with two combined fractional-period tests; A1 wording is changed in both the variable and the changelog; and the branch is now rebased onto merged |
juaristi22
left a comment
There was a problem hiding this comment.
Program Review
Source Documents
- Law: Childcare Payments Act 2014, section 15, plus sections 1 and 21.
- Companion implementation: policyengine-uk-data #473, current head
4a24766b653a80232ebf3b07a29faabf7975ecee. - Reviewed head SHA:
a6d605694139408f63427f7da6ce6a69dc166f93 - Mode: full patch review against base
9e4040540edc4bff07c1ef9e26d34dff5a74a9b4, plus incremental comparison from reviewed head6ebe6448efa4f085412c8d6e5df9cbbb54ff1aa3.
Critical (Must Fix)
- None.
Should Address
- [A4 NEW] The test preamble still calls the data input observed account use. The variable documentation and changelog now correctly say #473 supplies an HMRC activity-duration proxy, but the YAML comment says the dataset sets it from “observed account use.” Align that comment so future readers do not miss the proxy limitation. —
policyengine_uk/tests/policy/baseline/gov/hmrc/tax_free_childcare/tax_free_childcare.yaml:171 - [A5 NEW] The Towncrier fragment renders with a duplicated bullet. Remove its leading
-; Towncrier supplies the list marker. —changelog.d/tfc-spend-routed-share.added.md:1
Resolved Prior Findings
- [C1 RESOLVED] The share is now defined over the eligible period, the formula uses that same denominator, and a combined half-year × half-routed test returns £600 without double-discounting.
- [A1 RESOLVED] Variable documentation and changelog call #473's value an HMRC-derived duration proxy rather than observed routed expenditure.
- [C2 RESOLVED] The branch includes #1830's corrected 20%-of-gross formula and eligible-spend prorating.
- [C3 RESOLVED] The PR is based on the released #1830 implementation.
- [C4 RESOLVED] The model and data contracts are both person/child level.
- [A2/A3 RESOLVED] Unit, neutral default, clipping, boundary cases, sibling behavior, and the section 15 entity choice are covered.
Validation Summary
- Focused Tax-Free Childcare tests:
24 passed. - Ruff: passed on changed Python files.
- Towncrier: fragment is consumed, with the duplicated-bullet formatting issue above.
- Exact-head GitHub CI: all 8 checks passed; PR is mergeable.
Review Severity: APPROVE_WITH_COMMENTS
Next Steps
Clean up the two documentation-formatting nits; the formula and denominator contract are now sound.
… variable The floor was >=2.89.2 and the lock pinned exactly 2.89.2, so CI resolved a model without the variable and the build-time contract check failed. 2.93.0 is the release carrying PolicyEngine/policyengine-uk#1834. Relocking also pulls policyengine-core 3.27.1 -> 3.31.1 and policyengine-uk-data 1.56.14 -> 1.56.16, which the new model requires.
Adds the input the model was missing: Tax-Free Childcare tops up money paid through the account, not a family's whole childcare bill, and
childcare_expensesis annual.Defaults to 1, so this changes no household calculation and no built dataset. It is the mechanism only; the empirical value lives in PolicyEngine/policyengine-uk-data#473.
Based on
main, not stacked. It and #1830 both touch the contribution expression but do independent things, and combining them is what closes the Tax-Free Childcare calibration gap — see below.Why the empirical value is not here
A model parameter applies to single-household calculations as well as microsimulation. A population average would tell an individual family they receive a fraction of their statutory entitlement — wrong for them even where right on aggregate. This repo's sibling data package states the principle: parameters live there "to keep the country package as a purely deterministic rules engine".
An earlier attempt (#1833, closed) did put it in the parameter tree, and the giveaway was that it forced five household-level test expectations away from statutory values.
Per child, not per benefit unit
A Tax-Free Childcare account is held for one child only (Childcare Payments Act 2014 s.15(2)), and both
childcare_expensesandtax_free_childcareare person-level. A benefit-unit share could not represent siblings with different account use, and would shift totals where the per-child cap binds. There is a test for the sibling case.The value is clipped to 0–1 in the formula: data outside that range would otherwise produce a negative or supra-statutory award. The default of 1 is documented as a neutral all-spend-routed assumption, not a statutory requirement.
What the empirical value is, and is not
The companion PR sets it from HMRC's monthly and annual counts of children with used accounts:
sum(monthly) / annual unique / 12= 0.593 for 2024-25.That ratio measures the average fraction of months in which an account makes any provider payment. It is not value-weighted, and HMRC notes the date a payment is made need not be the date the childcare was received. Using it as a share of spending assumes spend is uniform across active months. It is a defensible proxy and a calibration adjustment, not a directly observed routed-spend share, and the companion PR assigns it as a constant because HMRC publishes no distribution behind the mean.
Combined effect
On
enhanced_frs_2024_25at 2024, againstpolicyengine-uk-data's targets, with #1830 (which corrects the gross/net rate error) also applied:Caseload is untouched throughout — the share governs how much of a child's spending is routed, not whether they claim.
Testing
Existing expectations keep their statutory values. Tests added for the default, a half share, a capped award, a zero share, clipping above one and below zero, and siblings routing different shares.