Give a place the geohash cell that contains its own centroid - #9
Merged
Merged
Conversation
The compact index assigns one owning place per cell, chosen by placetype rank and then population. Near a national border two towns routinely share a precision-5 cell, so the more populous one takes the whole cell - including the cell holding the other town's own centre. The point then resolves to the wrong country, which is the worst answer this library can give. Rank a place that is centred in a cell ahead of the population tie-break, still behind placetype rank: a county's home cell should not outrank a locality that covers it, because the locality is the more useful label for someone standing there. When two centroids share a cell the existing comparator decides, unchanged. A cover is the set of terminal nodes of a quadtree walk, so its cells never nest and a place has at most one cell containing its centroid. The claim is therefore a prefix test against the centroid geohash, exact for every cell the comparator is asked about, and it invents no cells the cover pass did not produce. It is also computable from compact_places.latitude/longitude, so append batches rank an already-written incumbent by the same rule and the result stays independent of country order. The dominant-city rollup could still delete the protected cell afterwards, so it now keeps the home cell of a town in another country. Rollups inside one country are untouched: absorbing suburbs into a metro is a deliberate labelling choice, while moving a town across a border is never right. Measured on a 7-country slice (us,mx,nl,de,at,ch,cz; WOF pinned by lockfile, precision 5, min population 5000, one country per append batch): locality centroids resolving to a different country fall from 35 to 9 of 13,192, with 26 fixed and none newly wrong. 3,032 centroids now resolve to the place itself; 2 no longer do. Lookup rows grow by 188 (0.04%) and the database by 16 KB (0.067%).
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: 213defa58e
ℹ️ 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 home-cell rule was applied in two places that each look at a single exact hash, so it left two gaps. The rollup's cross-border protection only ran in the descendant sweep, which skips hashes no longer than the promoted parent. When a foreign town's home cell was the parent cell itself, the promotion overwrote it before anything could protect it, and the town's own centre resolved across the border. The parent is now checked with the same rule before it is replaced; the promotion is refused rather than inverted, so cells the dominant city covers itself still answer with the dominant city. The comparator only ever ranks places that emitted the same hash, because candidates are looked up by exact hash. Cover cells do not nest within one place but do across places, so a town could own the coarse cell holding its centroid while a neighbour emitted a finer cell over the same point - and the runtime's longest-prefix walk answered with the neighbour. A reconciliation pass now replays the ownership rule down the chain of cells containing each centroid, using the same comparator, so placetype rank still decides first and a cell shared by two centroids still goes by population. It only reassigns cells that already exist, so the index cannot grow; the redundant-row pass usually drops the reassigned row entirely. Measured on the 7-country slice used for this branch (us/mx/nl/de/at/ch/cz, 13,163 locality centroids, precision 4->5, min population 5,000): both gaps fire zero times and the database is byte-identical, 467,585 rows either way. All 302 nested home-cell conflicts in that data are a region or county losing to a finer locality or county cell, which placetype rank already decides correctly; none are same-rank. The six remaining wrong-country centroids (Ebbs, Au, Kreuzlingen, St. Margrethen, Freilassing, Simbach a. Inn) all share a precision-5 cell with the winner, so population legitimately decides them. The fixes are proven by red-green specs rather than by the slice. The build summary now reports both counters so a world build can show whether either rule ever fires.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34bb968814
ℹ️ 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".
Measuring the branch against a batched build - one country per --append invocation, the shape the world database is actually built in - turned up three border towns whose centres crossed the border there but not in a single-batch build: Douglas AZ read as Agua Prieta, and Mittelberg and Riezlern in the Austrian Kleinwalsertal read as Oberstdorf. The cause is not the append merge. In each case a *county* dominated the parent cell and the rollup's descendant sweep deleted the locality's own home cell, because isCityPlacetypeCode lumps counties in with localities. Inside one batch that is merely a worse label; across batches it removes the only row defending the cell, and the neighbouring country written earlier keeps it. The rollup now only folds a place into a better- or equally-ranked label, which is what the cell comparator does anyway - it ranks placetype before everything else. Rollups between localities, which are what make a metro read as one city, are untouched. Two smaller fixes to the nested home-cell pass added last commit: Redundancy in the compact index was judged against any matching ancestor prefix, so a row was dropped whenever some coarser cell named the same place - even when a different place owned the cell in between, which the runtime's longest-prefix walk answers with instead. It is now judged against the nearest kept ancestor. The pass also read cell ownership as it mutated it, so a place that lost its own cell to a coarser claimant never carried its claim to the cells below, and the result depended on the order places were read. Claims are now collected before any is applied; each cell is still settled by a running maximum under the comparator, so the order no longer matters. Measured on the 7-country slice (us/mx/nl/de/at/ch/cz, min population 5,000, precision 4->5), built both as one batch and as seven appends: build centroids wrong-country lookup rows single batch, before 13,163 6 467,585 single batch, after 13,163 6 470,258 seven appends, before 13,192 9 467,772 seven appends, after 13,192 6 470,426 The batched build now agrees with the single-batch one, so the border answer no longer depends on how countries are partitioned into batches. The rollup guard is strictly additive: 2,673 new rows (+0.6%, +116 KB), every one of them owned by a locality, and not one existing cell changes hands. The other two fixes are inert on this data - zero shadowed rows across all seven batches, byte-identical databases - and are proven by red-green specs instead. The six remaining wrong-country centroids all share a precision-5 cell with the winner, so population decides them; only deeper precision moves those.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e9c722825
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7521d561ac
ℹ️ 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".
Two follow-ons to the rank guard, both settled the same way as the last round: build the 7-country slice as seven separate --append invocations, which is the topology the world database is built in, and count. The rank guard only covered the descendant sweep, so a county dominating the child cells could still replace a minor same-country locality at the promoted parent itself. A locality whose cover terminates at that parent has no descendant row for the sweep to protect, so its centroid resolved to the county - the exact downgrade the guard was added to prevent. The same rank check now runs before the parent is replaced. Nested home-cell claims were collected from the cells a place had won. When two same-rank places are centred in the same coarse cell the population tie-break gives it to one of them, and the loser's claim vanished even though it still covers the finer cells over its own centre and still outranks a third place that is not centred there. Claims now come from each place's own cover, which does not depend on what it won. Both are inert on this data: seven-append and single-batch databases are byte-identical to 2e9c722 - 0 rows changed, 0 added - and wrong-country stays at 6 batched, 6 single, 470,426 / 470,258 rows. They are proven by red-green specs, not by the slice. The build summary now reports the rollup and home-cell invariants, which is what settled the two findings not taken. Across all seven batches: foreign home cells kept below the promoted rank, 0; parent promotions refused on placetype rank, 0; promotions shadowing an ancestor home cell, 2 - both same-country and both over a lower-ranked ancestor, which is the locality-over-region rule working. On the finished index, 333 centroids sit under a longer row owned by another place; every one of them is a region or county shadowed by a better-ranked place, 0 cross-country and 0 where the shadowed place ranks same or better, in both topologies.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 641e0618db
ℹ️ 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".
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
The compact index gives each geohash cell exactly one owning place.
buildCompactLookupRowspicks the winner withcomparePlacesForHash: placetype rank, then population descending, then distance from the cell centre, then area, then id.Near a national border two towns on opposite sides routinely share a precision-5 cell (~5 km), and the more populous one takes the whole cell — including the cell that contains the other town's own centroid. The point then resolves to the wrong country, which is the most user-visible failure this library has.
Measured on a 7-country slice built from pinned WOF data (
us,mx,nl,de,at,ch,cz), 35 of 13,192 locality centroids resolved to a different country than the locality itself.Change
New
--home-cell-priority(default on;WOF_HOME_CELL_PRIORITY=0to disable).1. Comparator. A place that is centred in a cell wins it, ranked after placetype and instead of the population tie-break.
2. Dominant-city rollup.
promoteLocalityParentsByRegionCompetitionsuppresses minor localities inside a dominant city's parent cell, and would otherwise delete the cell the comparator just protected. It now keeps the home cell of a town in another country. Rollups inside one country are untouched, because absorbing suburbs into a metro is a deliberate labelling choice, while moving a town across a border is never right.Why the claim needs no precision qualifier
A cover is the set of terminal nodes of a quadtree walk (
buildGeohashCoverForGeometry), so its cells are disjoint and never nest: a place has at most one cover cell containing its centroid. Whenever the comparator is asked about a cell that a place actually covers, the prefix test against that place's centroid geohash is therefore exactly "this is its home cell" — no separate max-precision check is needed, and no cell the cover pass did not produce is ever invented. Coarser cells are only emitted when a polygon fully contains them, so two same-placetype places cannot both emit one; the qualifier would have been a no-op for real conflicts.It also could not have been implemented in the append path:
compact_placesstores no cover precision, and an incumbent's max precision depends on the placetype, area and options of the run that wrote it. The centroid, however, is stored, soresolveExistingCellConflictsevaluates the same claim for an already-written incumbent as for a challenger. The comparator remains a total order per cell, so the winner is the same no matter which country batch runs first — verified by a spec that builds both orders.Measurements
Slice:
WOF_COUNTRIES=us,mx,nl,de,at,ch,cz,WOF_DOWNLOAD=0with the pinned ref lockfile,WOF_MAX_PRECISION=5,WOF_COUNTY_MAX_PRECISION=5,WOF_MIN_POPULATION=5000, one country per append batch (so every border pair goes through the append-merge path). Same inputs, same binary, only the flag differs.The five reported cases, probed at each town's own centroid
Simbach is not fixed, by design: its centroid (48.264171, 13.018799) and Braunau's (48.252241, 13.043426) are 2.25 km apart and fall in the same precision-5 cell
u2968. Two centroids in one cell fall through to the population tie-break, and Braunau is larger (16,403 vs 9,979). They only separate at precision 6, so this case needs a deeper index, not a different ownership rule.Slice-wide sweep (every locality centroid, resolved through
geocoder.reverse)Six of the nine remaining wrong-country cases are the Simbach class — twin towns sharing one precision-5 cell (Kreuzlingen/Konstanz, Freilassing/Salzburg, St. Margrethen and Au/Lustenau, Ebbs/Kiefersfelden, Simbach/Braunau). The other three (Douglas/Agua Prieta, Riezlern and Mittelberg/Oberstdorf) have no cell of their own to claim in the cover.
Cost
Where it made something worse
Two localities out of 13,192 (0.015%) resolved to themselves before and no longer do: Brockport (NY) and Greece (NY), both now answering "Monroe" — Monroe County. Neither is a country error. The cause is second-order: with per-cell ownership slightly redistributed, the dominant-city rollup's inputs shift, and the parent cell
dr8t(which the rule-off build left un-rolled, with 32 child rows) is now rolled up to the county, suppressing its minor-locality children. Worth noting for a follow-up that the rollup treats a county as a candidate "dominant city" at all —isCityPlacetypeCodeincludescounty— which is what turns a locality label into a county label here. That behaviour predates this change.Tests
spec/boundary_home_cell_spec.js(new, 6 specs) spawns the CLI over synthetic border geometry in temp dirs and asserts through the same longest-prefix lookup the runtime uses:expect(home.country_id).toEqual('US'));--home-cell-priority falserestores the old behaviour;spec/boundary_builder_merge_spec.jsgains a spec for the append path: a legacy row centred in a contested cell keeps it against a more populous appended city, while still losing the rest of the strip. The existing "higher population takes a cell from a legacy row" fixture had its legacy centroid moved out of the contested cell — it sat inside it by coincidence, so the case it means to cover (population deciding) is now actually what it exercises.Red/green proof: with the two
scripts/changes stashed and the specs kept, the headline spec fails asExpected 1001 to equal 2001andExpected 'MX' to equal 'US'— the wrong-country bug itself — and the append-order spec fails in both orders. Restored: 42 specs, 0 failures.