Skip to content

Drop ons/council_tax_band_d/{code} target emission (#483) - #484

Open
vahid-ahmadi wants to merge 2 commits into
mainfrom
fix/483-drop-band-d-target
Open

vahid-ahmadi wants to merge 2 commits into
mainfrom
fix/483-drop-band-d-target

Conversation

@vahid-ahmadi

@vahid-ahmadi vahid-ahmadi commented Sep 18, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #483.

Decision and provenance

A Band D council tax amount is a rate (£ per Band D dwelling), not an additive household control. The loss matrix is an additive objective — every target is compared against a weighted sum over households. Binding a ~£2,500 per-dwelling rate into it is category-wrong: not as a sum (which scales with the number of households in the LA), and not as a mean (still a rate inside an additive objective).

This is not a fresh judgement call. Microcosm reached the same verdict independently. uk_data_target_parity.json concern local_council_tax_band_d_rate is:

  • classification: non_linear
  • status: reviewed_exclusion
  • reason: "Band D currency amounts are per-rate, not additive household controls."
  • verdict basis: "Keep excluded until a rate-aware non-linear objective exists."

So the resolution is to drop the emission, not to add a mean-returning compute route.

Why this is latent-bug removal, not a behaviour change today

council_tax_band_d_amount does not exist in policyengine-uk, so build_loss_matrix.py::_compute_simple_gbp raises KeyError, returns None, and the caller continues — all 350 of these targets are silently discarded today. The defect activates the moment policyengine-uk gains that variable (PolicyEngine/policyengine-uk#1855 adds it): _compute_simple_gbp would then sum it against a ~£2,500 level.

Removed

  • The per-LA Target(name=f"ons/council_tax_band_d/{code}", variable="council_tax_band_d_amount", ...) loop in get_targets(), replaced by a comment block explaining the rate-vs-additive reason, citing Microcosm's local_council_tax_band_d_rate reviewed_exclusion and this issue, so it does not get helpfully re-added.
  • Now-dead helpers, verified unused by grep: _year_for_band_d, _ref_for_band_d, and the constants _ENGLAND_REF, _SCOTLAND_REF (the identically-named _SCOTLAND_REF in voa_council_tax.py is a separate symbol and is untouched), _YEAR_BAND_D_WALES, _YEAR_BAND_D_SCOTLAND.
  • test_band_d_targets_use_gbp_unit (vacuous once nothing matches).

Deliberately kept

  • band_d_amount in storage/la_council_tax.csv — needed as data for liability computation. git diff origin/main -- policyengine_uk_data/storage/la_council_tax.csv is empty: the CSV is byte-identical to main. The loader is untouched, and a new test asserts the column is still present and populated for 350 LAs.
  • voa/council_tax/{code}/{band} band-count targets — 2,563, unchanged. These are counts, counts are additive, and Microcosm has them ported_local_declared and actively bound. Not touched.
  • housing/council_tax_net/{code} — 318, unchanged.
  • _YEAR_BAND_D_ENGLAND kept: still used, because the England net council-tax requirement derives from the 2026-27 Band D level.

Target counts, before and after

main this branch
ons/council_tax_band_d/* 350 0
voa/council_tax/* 2,563 2,563
housing/council_tax_net/* 318 318
total 3,231 2,881

Tests

New/changed in test_la_council_tax_targets.py:

  • test_no_band_d_amount_target_is_emitted — asserts no emitted target name starts with ons/council_tax_band_d/ (or contains council_tax_band_d at all).
  • test_band_d_amount_column_still_loadable — the CSV column survives.
  • test_band_count_target_count_matches_csv — now also pins the count at 2,563 so a future change to the Band D surface cannot quietly disturb the band-count family.

Full suite, repo's normal invocation:

$ uv run --python 3.13 pytest -q -m "not slow"
521 passed, 108 skipped, 5 deselected, 1 xfailed, 3 warnings in 89.67s

ruff check . reports 61 errors on this branch. Measured per-file rather than assumed: both la_council_tax.py and test_la_council_tax_targets.py report 0 errors on origin/main and 0 on this branch (via git show origin/main:<f> | ruff check --stdin-filename <f> -), so all 61 are pre-existing and live in files this PR does not touch.

Not done / follow-ups

🤖 Generated with Claude Code

A Band D council tax amount is a rate (£ per Band D dwelling), not an
additive household control. The loss matrix is an additive objective, so
binding a ~£2,500 per-dwelling rate into it is category-wrong — neither
as a sum nor as a mean.

Microcosm adjudicated the same question independently: its
uk_data_target_parity.json concern `local_council_tax_band_d_rate` is
status `reviewed_exclusion`, classification `non_linear`, reason "Band D
currency amounts are per-rate, not additive household controls", to stay
excluded "until a rate-aware non-linear objective exists".

Removed: the per-LA Band D amount Target emission (350 targets, all of
which were silently discarded today because `council_tax_band_d_amount`
does not exist in policyengine-uk) and the now-dead `_year_for_band_d` /
`_ref_for_band_d` helpers and `_ENGLAND_REF` / `_SCOTLAND_REF` /
`_YEAR_BAND_D_WALES` / `_YEAR_BAND_D_SCOTLAND` constants.

Kept: `band_d_amount` in storage/la_council_tax.csv (byte-identical) and
loadable, the `voa/council_tax/{code}/{band}` band-count targets (2,563,
unchanged — counts are additive and Microcosm binds them as
ported_local_declared), and the `housing/council_tax_net/{code}` targets
(318, unchanged).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The net council-tax target reads total_council_tax_net straight from
  the CSV; the taxbase x Band D derivation happens upstream at CSV build
  time. Reword the comment accordingly.
- Rename changelog.d/483.md to 483.removed.md so towncrier picks up the
  fragment type.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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.

Approving. Dropping the emission is the right shape for the Microcosm register, and it answers the question on #483: no, fencing in place is not needed. The local_council_tax_band_d_rate concern keeps covering targets/sources/la_council_tax either way, and its fence assumed exactly this state. One correction on our side: the fence's origin line says uk-data "did not wire them into the additive LA loss matrix", which was wrong until this PR (the target existed and was silently dropped). We will update that evidence to cite #483 and this PR once it lands, and re-pin the inventory sha256s for la_council_tax.py and test_la_council_tax_targets.py in Microcosm's uk_data_target_inventory.json.

Sequencing: please merge this before any policyengine-uk pin bump that includes PolicyEngine/policyengine-uk#1855.

Nits, none blocking:

  • The uv.lock change (the package's own version line, 1.56.16 to 1.57.3) is not part of this fix. Keep it only if main's lock is behind pyproject.toml, otherwise drop it from the diff.
  • The == 350 and == 2563 pins will need touching on every CSV refresh. Fine as a deliberate tripwire; the refresh commit should say so.
  • The conflict with #482 is real (same docstring region, same test file); whichever lands second keeps both paragraphs.

🤖 Generated with Claude Code

This branch has not been deployed

No deployments
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.

LA Band D target is silently dropped now, and will compute as a sum-vs-level mismatch once council_tax_band_d_amount exists

2 participants