perf: one cell→country resolution per render + memoized metadata Series (#251) - #257
Merged
Merged
Conversation
…exed Series (#251) The adapter answered 'which country is each cell in?' twice per map (once per label) and every lookup re-sorted the 66,205-row cell→country table because only the raw DataFrame was cached. Now: get_labels_for_index resolves ONCE and derives both isoab and name from the same resolution (the single accessors remain as thin wrappers — public API unchanged); the indexed lookup Series memoise beside the raw tables, and _reset_metadata_cache clears both. Behavior-identical by construction and by test: same labels (incl. the '{country_id} - {name}' shape), same NaN degradation, same aggregated warning (once per resolution — the duplicate per-render warning noise is gone), same all-unknown raise. Measured on the real bundle at one global month: 73.1 ms → 52.9 ms per adapter call (-28%); the label-resolution share roughly halved, multiplied across ~21 renders/target per report. New tests: combined-accessor equivalence vs the singles (CM+PGM, incl. unknown entities), a spy proving the adapter triggers exactly one resolution, and cache hygiene (both caches populate and clear together). Test doubles updated to mock the combined accessor. 405 tests green. Co-Authored-By: Claude Fable 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.
Closes #251 (the epic #230 efficiency follow-up).
What
get_labels_for_index(new): both labels —isoab+name— from ONE cell→country resolution; the single accessors become thin wrappers (public API unchanged); the adapter makes one call and one merge instead of two of each.set_index(...)lookup Series cache beside the raw tables;_reset_metadata_cacheclears both.Measured (real bundle, 66,205 rows, one global month)
73.1 ms → 52.9 ms per adapter call (−28%) — multiplied by ~21 renders/target per report.
Behavior-identity
Same labels, NaN degradation, warnings (now once per resolution instead of twice per render), and all-unknown raise. Guarded by: the untouched existing accessor/contract/seam suites, plus three new tests — combined-vs-singles equivalence, a spy pinning exactly one resolution per adapter call, and cache hygiene. Test doubles updated to the combined accessor. Adapter CIC updated (also fixed a pre-existing stale line claiming the accessors 'own the viewser fetch' — bundled since C-22).
One honest nuance: the thin wrappers compute both labels and select one, so a lone single-accessor call does marginally more work — only the CM line graph uses one alone, over the 213-row country table; negligible, and it keeps the resolution logic single-source.
Ritual
review-diff: CLEAN. ship-it: ruff clean, 405 passed (2 skipped, 1 xfailed), docs validator green.
🤖 Generated with Claude Code