Skip to content

Add tax_free_childcare_spend_routed_share as a neutral-default input - #1834

Merged
vahid-ahmadi merged 1 commit into
mainfrom
tfc-payment-active-input
Aug 28, 2026
Merged

Add tax_free_childcare_spend_routed_share as a neutral-default input#1834
vahid-ahmadi merged 1 commit into
mainfrom
tfc-payment-active-input

Conversation

@vahid-ahmadi

@vahid-ahmadi vahid-ahmadi commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

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_expenses is 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_expenses and tax_free_childcare are 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_25 at 2024, against policyengine-uk-data's targets, with #1830 (which corrects the gross/net rate error) also applied:

spending caseload
baseline 1.87× 1.02×
routed share only 1.25× 1.02×
#1830 only 1.60× 1.02×
both 1.02× 1.02×

Caseload is untouched throughout — the share governs how much of a child's spending is routed, not whether they claim.

Testing

policyengine-core test policyengine_uk/tests/policy/baseline/gov/hmrc/tax_free_childcare -c policyengine_uk
81 passed

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.

@vahid-ahmadi

Copy link
Copy Markdown
Collaborator Author

Rebased onto main and retargeted. It was stacked on #1830, but the two are independent — #1830 multiplies the contribution by eligible_fraction (part-year eligibility), this multiplies it by routed_share (share of spend through the account). Neither needs the other, and stacking meant this could not merge while #1830 was under discussion. 372 policy tests pass against main.

@juaristi22 juaristi22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Program Review

Source Documents

Critical (Must Fix)

  1. [C1] The new tests and target rationale still treat gross provider spend as a parent deposit. childcare_expenses is a Person input documented as the total amount spent on childcare, and the contribution.rate parameter is 0.2 of total household-plus-government contributions. But tax_free_childcare.py:40-42 computes gross * 0.2 / 0.8 = 25% of gross; the new tests at tax_free_childcare.yaml:119-138 therefore 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.

  2. [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

  1. [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_expenses and tax_free_childcare are 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

  2. [A2] Complete the input contract. This dimensionless share has no unit = "/1" (runtime metadata reports None), 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

  3. [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_periods and this share from the same activity-duration series, which would double-scale spend. — tax_free_childcare.yaml:115-150

  4. [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_fraction to contribution. Resolve the overlap deliberately and rerun household, statutory, and aggregate validation after #1830 lands.

Suggestions

  1. [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.

@vahid-ahmadi
vahid-ahmadi force-pushed the tfc-payment-active-input branch from 7527c50 to 1d92052 Compare August 28, 2026 11:33
@vahid-ahmadi

Copy link
Copy Markdown
Collaborator Author

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: contribution/rate.yaml describes 0.2 as "the government contribution to childcare expenses (as a percentage of total household and government contributions)", and childcare_expenses is "Total amount spent on childcare". So rate / (1 - rate) applied the deposit-side 25% to a gross-side base. I had filed this as #1832 because I could not settle the input contract from FRS documentation; your Act s.1(4) / s.21 and TFC35300 citations settle it.

Your prediction was right on the numbers. With both changes on enhanced_frs_2024_25 at 2024:

spending caseload
baseline 1.87× 1.02×
routed share only 1.25× 1.02×
#1830 only 1.60× 1.02×
both 1.02× 1.02×

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 Person. You are right that the account is per child under s.15(2) and that both consuming variables are person-level. Added a test with siblings routing different shares, which the benefit-unit version could not represent.

A2 — contract. Added unit = "/1", clipping to 0–1 in the formula, and tests for above-one and below-zero. The default is now documented as a neutral assumption rather than a statutory requirement.

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 eligible_declaration_periods and this share from the same activity series, spend would be scaled twice. Nothing does that today and the two are documented as distinct quantities, but it is worth watching.

A4 — stack. The body was stale; I rebased onto main earlier and did not update it. Corrected, with the reason the two are independent.

S1 — release order. Agreed, and noted on #473: the model change has to ship first, since older versions do not declare the input.

@vahid-ahmadi

Copy link
Copy Markdown
Collaborator Author

Merge-order note: this and #1830 are each mergeable against main but conflict with one another, since both edit the contribution expression in tax_free_childcare.py. Whichever merges second needs a small rebase — the two changes compose rather than compete.

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 enhanced_frs_2024_25 at 2024, with the routed share set to HMRC's 0.593:

spending caseload
baseline 1.87x 1.02x
#1834 only 1.25x 1.02x
#1830 only 1.60x 1.02x
both 1.02x 1.02x

@juaristi22 juaristi22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Program Review

Source Documents

Critical (Must Fix)

  1. [C1 STILL OPEN] The standalone head still applies 25% to gross childcare spend. The formula divides p.rate by 1 - 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
  2. [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 assigns tax_free_childcare_spend_routed_share to pe_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 #473 policyengine_uk_data/datasets/frs.py:1434

Should Address

  1. [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.

@vahid-ahmadi

Copy link
Copy Markdown
Collaborator Author

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 # 40_000 * 0.5 * 0.25 = 5_000 comment. Running them against merged code gives seven failures: three expectations #1830 already rewrites, plus four of the new routing cases (500→400, 250→200, [500, 250]→[400, 200], and the clip-at-one case 500→400).

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.

vahid-ahmadi added a commit to PolicyEngine/policyengine-uk-data that referenced this pull request Aug 28, 2026
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
@vahid-ahmadi
vahid-ahmadi force-pushed the tfc-payment-active-input branch from 1d92052 to 2dbbc41 Compare August 28, 2026 12:45
@vahid-ahmadi
vahid-ahmadi changed the base branch from main to fix-tfc-part-year-prorating August 28, 2026 12:45
@vahid-ahmadi

Copy link
Copy Markdown
Collaborator Author

All three addressed.

C1 — restacked on #1830. You were right that leaving this on main kept the 25%-of-gross formula and the expectations that encode it, and that the two heads conflicted in both the formula and the tests. This branch now sits on fix-tfc-part-year-prorating, so the formula is #1830's corrected expense * eligible_fraction * routed_share * rate at 20% of gross, and all seven routing expectations are re-derived: 500→400, 250→200, the clip-at-one case 500→400, and the siblings case [500, 250]→[400, 200]. The capped, zero-share and negative-share cases are unchanged, as they should be. 380 tests pass across gov/hmrc.

C3 — the companion data PR wrote the wrong entity. Good catch, and it was a break I introduced: I changed this variable to Person in response to your A1 and did not update PolicyEngine/policyengine-uk-data#473, which was still assigning it to pe_benunit. The published input would not have populated the variable it feeds. Fixed there — moved to pe_person, alongside the other person-level stochastic assignments, with the s.15(2) reasoning in a comment.

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 childcare_expenses being gross, not on the fit.

@vahid-ahmadi
vahid-ahmadi force-pushed the fix-tfc-part-year-prorating branch from a345ecf to 25a588f Compare August 28, 2026 13:22
@vahid-ahmadi
vahid-ahmadi force-pushed the tfc-payment-active-input branch from 2dbbc41 to 6ebe644 Compare August 28, 2026 13:22

@juaristi22 juaristi22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. [C1] The new denominator documentation is mathematically backwards and makes fractional eligibility double-discount the same months. Lines 18–22 define routed_share as the share of whole-year spending routed through the account. But the formula at lines 71–73 also multiplies by eligible_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 returns annual_spend * 0.5 * 0.25, only half of the routed amount. The correct factorizations are either annual_spend * whole_year_routed_share or annual_spend * eligible_fraction * routed_share_conditional_on_eligible_period. Companion data #473 explicitly supplies active 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 because eligible_declaration_periods is 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

  1. [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-level Person entity.
  • [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 passed on 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.

@vahid-ahmadi
vahid-ahmadi force-pushed the tfc-payment-active-input branch from 6ebe644 to d257863 Compare August 28, 2026 13:39
@vahid-ahmadi

Copy link
Copy Markdown
Collaborator Author

@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 eligible_fraction. That reasoning runs the wrong way: the second multiplication is precisely why a whole-year share double-discounts. Your worked case reproduces:

routed share contribution base correct?
whole-year denominator 3/12 = 0.25 12,000 × 0.5 × 0.25 = 3,000… ×0.2 = 300 no, half
eligible-period denominator 3/6 = 0.5 12,000 × 0.5 × 0.5 = 6,000… ×0.2 = 600 yes

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 tax_free_childcare_eligible_declaration_periods returns eligible * declaration_periods_per_year and so is 0 or 4 — the eligible fraction is 1 for everyone who receives anything, and the two denominators coincide. I have written that into #473 next to the derivation, with the condition under which it stops holding, since the model exposes fractional periods even though the data never produces them.

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 policyengine_uk the venv had installed rather than the working tree, unless the path was forced — the policyengine-core entry point does not put the working directory first. That bit me here and reported 15 spurious failures against correct code.

Base automatically changed from fix-tfc-part-year-prorating to main August 28, 2026 13:53
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
@vahid-ahmadi

Copy link
Copy Markdown
Collaborator Author

Rebased onto main now that #1830 has merged.

#1830 was squash-merged, so its commit was not an ancestor of main and this branch was carrying a stale copy of it. The diff against main had picked up three artefacts that had nothing to do with this change: it removed 7 lines from CHANGELOG.md, re-added changelog.d/tfc-part-year-prorating.fixed.md which the release had already consumed, and downgraded pyproject.toml from 2.92.2 back to 2.92.1.

git rebase --onto origin/main 25a588f7 drops the duplicated commit. The diff is now the four files this PR is actually about:

changelog.d/tfc-spend-routed-share.added.md          |   1 +
tests/.../tax_free_childcare.yaml                    | 121 +++++++++++++++
variables/.../tax_free_childcare.py                  |  13 ++-
variables/.../tax_free_childcare_spend_routed_share.py |  30 +++

No duplicated YAML cases (24 distinct test names), and 382 gov/hmrc policy tests pass against merged main. CI should now attach properly — it had none before, because the base was a branch rather than main.

@vahid-ahmadi
vahid-ahmadi force-pushed the tfc-payment-active-input branch from d257863 to a6d6056 Compare August 28, 2026 13:56
@vahid-ahmadi

Copy link
Copy Markdown
Collaborator Author

@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 main, which also removed a stale CHANGELOG deletion and a 2.92.2→2.92.1 version downgrade it had been carrying. Are you happy with these?

@juaristi22 juaristi22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 head 6ebe6448efa4f085412c8d6e5df9cbbb54ff1aa3.

Critical (Must Fix)

  • None.

Should Address

  1. [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
  2. [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.

@vahid-ahmadi
vahid-ahmadi merged commit 12c2520 into main Aug 28, 2026
8 checks passed
@vahid-ahmadi
vahid-ahmadi deleted the tfc-payment-active-input branch August 28, 2026 14:41
vahid-ahmadi added a commit to PolicyEngine/policyengine-uk-data that referenced this pull request Aug 28, 2026
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants