Keep counties out of the dominant-city rollup - #10
Merged
Merged
Conversation
A parent cell rolled up to its dominant city could pick a county, because isCityPlacetypeCode() treats county as city-like. The metro then reads as its county's name and the minor localities under it lose their own cells. Rank candidates as before, but refuse to roll a parent cell up to a placetype that may not name a metro. --dominant-city-placetypes (default locality,localadmin) selects the eligible set; adding county restores the previous behaviour.
Three specs on the Rochester shape: a county that outpopulates the towns inside it, two towns holding their own child cells, a region as fallback. Against the previous builder the first two fail (the parent cell reads 8002, the county, and both towns lose every cell), and the third one -- a genuine city rollup with a county owning a cell beside it -- passes unchanged.
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 100ecd6121
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The rollup has two ways to hand a parent geohash cell to a child: winning the dominant-city competition, and being the only city-like owner in that cell. Only the first consulted --dominant-city-placetypes, so a county that owned a parent cell alone and cleared --parent-locality-min-share still took the cell under the default locality,localadmin -- the exact outcome the option exists to prevent. Both paths now go through localityMayTakeOverParent(), which pairs the placetype check with the child-share threshold so the two cannot drift apart. selectDominantLocalityId() drops its own copy of the placetype check and goes back to being a pure population ranker; a candidate rejected by the gate is still never replaced by the runner-up. Measured on the PR's slice (us,mx,nl,de,at,ch,cz at precision 5, counties included, min population 5000): 18 parent cells were leaking through the single-candidate path -- 9 in MX, 7 in AT, 2 in US (cells in New Hampshire reading "Grafton" and "Oxford") -- and now fall back to their region. Lookup rows 494,068 -> 494,550 and file bytes 25,591,808 -> 25,600,000, so the rule costs +5.33% over a county-eligible build rather than the +5.29% measured before. Probing the centre and four quarter points of all 18 cells: 77 of 90 answers unchanged, 13 county -> region, none lost. A build with county opted back in is byte-identical to before this commit.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
Problem
promoteLocalityParentsByRegionCompetitionrolls a parent geohash cell up to the place that dominates its children, so a metro reads as one city instead of a patchwork of suburbs. The candidate set comes fromisCityPlacetypeCode, which countscounty(PLACETYPE_CODES.county = 3) as city-like — andselectDominantLocalityIdthen ranks purely on population.A county almost always outpopulates every town inside it, so it wins that ranking and the rollup does two things at once: the parent cell takes the county's name, and
suppressMinorLocalitiesdeletes the child cells of every minor locality under it. Points that had their own town's cell fall back to the parent and answer with a county name where a locality name is expected.The dominant-city rollup exists to make a metro read as one city. A county is not a city.
This is what PR #9 hit from the other side: with per-cell ownership slightly redistributed,
dr8t(Rochester NY) rolled up to Monroe County (748,482) and Brockport (8,366) and Greece (14,519) started answering "Monroe". That report is what prompted this change, but the behaviour predates it — onmastertoday, Rochester, NH (29,752) answers "York", a county in a different state.Change
selectDominantLocalityIdstill ranks every competitor for the parent cell on population, unchanged — a major county in the group still blocks a rollup exactly as before. What changes is the verdict when the winner is a placetype that may not name a metro: the rollup is abandoned, and every child keeps the cell it owns.It deliberately does not fall through to the runner-up city. That city lost the same population competition; promoting it would name the parent cell after a place the comparator already rejected, which is the "muni renamed to the nearest big city" failure in a different costume.
Everything else about counties is untouched:
comparePlacesForHashon its own merits (placetype rank, population, distance);isCityPlacetypeCodeat the two incumbent checks);New
--dominant-city-placetypes(defaultlocality,localadmin;WOF_DOMINANT_CITY_PLACETYPESfor the WOF wrapper). Passinglocality,localadmin,countyreproduces current builds exactly. A placetype that is not city-like at all (region) is rejected rather than silently ignored, because accepting it would look like it did something.localadminstays eligible: where it is included at all (--include-localadmin true, off by default) it is the municipality that carries the city's name — the Guatemala City case. These builds do not include localadmin, so the slice cannot separate it fromlocality; excluding it would be an untested guess.Measurements
Slice:
WOF_COUNTRIES=us,mx,nl,de,at,ch,cz,WOF_DOWNLOAD=0against the pinned ref lockfile,WOF_MAX_PRECISION=5,WOF_COUNTY_MAX_PRECISION=5,WOF_MIN_POPULATION=5000, all seven countries in one batch. Same inputs, same binary, onlyWOF_DOMINANT_CITY_PLACETYPESdiffers. Answers come fromgeocoder.reversethroughsrc/index.js, so they are what a caller sees, not what the table holds.Label quality — every locality centroid in the slice (13,163 of them)
Nothing regressed into a county label, and three of the wrong-country cases went away as a side effect -- a town answering with a foreign county's name is both errors at once (Somerton AZ read Mexicali, Rosarito BC read Tijuana, the county).
A sample of what changes, at each town's own centroid:
Cost
The rows come back because a rollup that no longer happens is a set of child cells that no longer gets deleted. Extrapolating the ratio to the 24.5 MB world asset puts it near 25.8 MB — well inside the 50 MB ceiling.
Where it could have read worse, and what actually happens
A rollup does not only rename the parent cell, it creates that row when no polygon covered the parent cell outright. Skipping the rollup therefore removes 875 precision-4 rows (817 vanish, 58 fall back to a region owner). Those rows answer only for points that no precision-5 cell covers, so they are exactly where this change could degrade a label.
Probing the centre and four quarter points of all 875 changed cells (4,375 points, both builds):
Not one point loses an answer, and the coarser cases are 13 out of 4,375 (0.3%) against 453 that get finer. The Hawaii cells are typical of the win:
8e8wanswered "Maui" for every point in it and now answers Kihei, Lahaina and Kahului.The reported case, stacked on #9
The Brockport/Greece report came from a build with
--home-cell-priorityon, so the same pair was built again onfeat/home-cell-ownershipwith this commit cherry-picked. Same slice, same recipe.The rest of that pair matches the
masternumbers closely — county-named localities 485 -> 1, +473 resolving to themselves, 0 newly county, +5.37% file size — so the two changes are independent: #9 decides which place owns a cell, this decides whether a county may name a whole parent cell.On
mastertoday Brockport and Greece already answer correctly, which is why the fix has to be measured on both branches: the county rollup is the standing bug, and #9 only changed which parent cells it happened to reach.Tests
Four specs in
spec/boundary_builder_spec.js, on the Rochester shape: a county that outpopulates every town in it (748,482), two towns holding child cells of their own (8,366 and 14,519), and a region as the fallback owner of the parent cell.--dominant-city-placetypes locality,localadmin,countyreproduces the old rollup on the same fixture (parent reads as the county, both towns lose every cell), which is also what proves the fixture reproduces the bug;region) is rejected rather than silently ignored.Red/green: against the previous builder the first two fail as
Expected $[0].place_id = 8002 to equal 8001(the parent cell reading as the county) plusExpected 0 to be greater than 0for both towns, and the third passes unchanged. With the change: 39 specs, 0 failures.Downstream note
Citylap bundles a database built from this script whose build id ends in
majorrollup. A rebuild with this default would grow the 24.5 MB asset by roughly 5% and change the label for the cells listed above; existing stored rows are unaffected, since the app never re-geocodes history.