Re-fit TOP's tobacco band for #734, and stop asserting float equality - #737
Merged
Merged
Conversation
Two tests in test_nowcast_product_taxes.py have been red on nowcast since #691 merged, but #691 did not cause them: a dispatched CI run on 7827a9e, nowcast immediately before #691, fails with the same two tests and a bit-identical -26278.087320966762. The cause is #734 x #733. #733's last green run (26acafc) did not contain #734 -- `git merge-base --is-ancestor a57b0ca 26acafc` is false -- so the -26,153 band was fit on pre-#734 data. #734 then remapped the Census NAICS 2022 activities onto the goods Crosswalk, which moves the Trade FBS, purchaser_base and so the residual's annual shares. Each PR was green alone; neither was ever built with the other. Re-fit tobacco to -26,278, 0.48% off the old centre against a 0.38% band. The 2017 anchors -- the control totals, NAMED_TOTAL_2017, RESIDUAL_TOTAL_2017 and the 391,162 trade-level split -- all still pass, so the level is intact and it is the shares that moved. The divergence also grew rather than fell, which the test's own docstring names as the safe direction. test_the_two_levels_add_back_to_the_column was a separate, latent bug. producer_level is defined as TOP - trade_level, so .equals asserted that (a - b) + b == a bit-exactly, which floating point does not guarantee; it had been passing on luck. Compared with rel=1e-12 instead. Grouped the four fitted bands into two dict comparisons so a future source change reports every band that moved in one run rather than only the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WesIngwersen
added a commit
that referenced
this pull request
Aug 27, 2026
…738) #737 re-fit tobacco but merged before its own CI reported, so it fixed two of the three bands #734 moved. The run that finished afterwards (33066747660) shows the third: the 2024 half-sum share is 0.0733 against 0.077 +/- 0.003. The other bands are confirmed unmoved by the same run - tobacco holds at -26,278 and 5241XX 2020 at 17,791, and the 2020 share holds at 0.063. Grouping the four into two dict comparisons is what surfaced all of them at once instead of one per 8-minute round. The fall needed checking rather than re-fitting, because the docstring names a fall as the signal that the residual stopped moving. It has not: the frozen-share value for 2024 is 5.8%, and 7.3% is a long way above it, while tobacco's own gap grew over the same change. #734 re-allocated within a residual that still moves. Recorded that reasoning in the docstring so the next fall is measured against 5.8% rather than against whatever the band happens to say. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
nowcasthas been red since #691 merged. #691 is not the cause.A dispatched CI run on
7827a9e0— nowcast immediately before #691 — fails with the same two tests and a bit-identical value,-26278.087320966762(run 33065794809).What actually happened
#733's last green run was on its own branch,
26acafc5, and that branch did not contain #734:#734 merged first, #733 second. The
-26,153band was fit on pre-#734 data. #734 then remapped the Census NAICS 2022 activities onto the goods Crosswalk → Trade FBS →purchaser_base→residual_share_for_year→top_column. Each PR was green on its own branch and neither was ever built with the other.The two failures
test_the_two_constructions_diverge_where_the_named_lines_do— a fitted band. Tobacco's 2024 gap is-26,278against-26,153 ± 100: out by 125, or 0.48% against a 0.38% band. Re-fit to-26,278.This is the shares moving, not the level. Every 2017 anchor in the file still passes — the control totals,
NAMED_TOTAL_2017,RESIDUAL_TOTAL_2017, and the 391,162 trade-level split. The divergence also grew, which the test's own docstring names as the safe direction; a fall would have meant the residual stopped moving.test_the_two_levels_add_back_to_the_column— a separate latent bug, unrelated to either PR.producer_levelis defined asTOP - trade_level, so.equals()asserted(a - b) + b == abit-exactly. Floating point does not guarantee that; CI's own values show1111A0agreeing to every printed digit and differing in the last ulp. It had been passing on luck. Now compared withrel=1e-12.Also
Grouped the four fitted bands into two
pytest.approxdict comparisons, so the next time a source underpurchaser_basemoves them, one run reports every band that moved instead of only the first.