Repository navigation
Persist whether a stored centroid lies inside its own geometry - #11
Conversation
Normalization refuses a home-cell claim to a centroid that falls outside its own shape - the bounding-box fallback for a concave, holed, or multipart polygon can easily land in a notch or a hole. That decision lived only in memory. cellCandidateFromCompactRow() rebuilds an append incumbent from its stored coordinates, so every batch after the one that wrote the place handed the claim straight back, and a challenger genuinely centred in a contested cell could lose it to a point that is not inside the incumbent at all. The decision is now stored in a nullable compact_places.centroid_inside and read back on append. NULL means "not recorded" and keeps the pre-upgrade behaviour of trusting the coordinates, so existing databases are unchanged until they are rebuilt; older schemas gain the column in place with ALTER TABLE. Readers never select it. Catalogued as schema generation 5 in COMPATIBILITY.md with its own permanent reader fixture, which pins all three states a row can carry - 1, 0 and the NULL every pre-upgrade row keeps. This is live on real data, not a hypothetical: 52 of 3,354 places in ch/at/nl/cz have a centroid outside their own polygon (32 CH, 18 NL, 1 AT, 1 CZ), and the world database is built as 26 --append batches, so 25 of them see those incumbents through this path. Red-green spec: a horseshoe whose bbox midpoint lands in its slot, and a town in the next country genuinely centred in the cell that midpoint falls in - a cell the horseshoe really does clip, so it legitimately emits it. Ignoring the stored flag fails with Expected 9001 to equal 9002 / Expected 'US' to equal 'MX'.
|
@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: 5f70570bb5
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c029da5b13
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc8ba6f0f5
ℹ️ 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".
| if (!answeredAt) continue | ||
|
|
||
| for (var finer = answeredAt + 1; finer <= deepest; finer++) { | ||
| var hash = place.homeGeohash.slice(0, finer) |
There was a problem hiding this comment.
When an existing database contains geohashes deeper than 12—supported because the builder and reader accept --max-precision values above 12—homeGeohash remains capped at HOME_CELL_PRECISION, so slicing it here repeatedly produces the same 12-character hash instead of the required descendants. Even after the stored-depth fix, this fresh evidence means an append whose home cell is shadowed at precision 13 or deeper can still report zero; generate hashes beyond the cached length as ownsHomeCell() already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #12 — the detector moved there so #11 stays single-purpose. Hashes past the cached home geohash are now encoded from the centroid as ownsHomeCell() does, rather than sliced out of a 12-character string. No spec: it is unreachable at any precision this project builds at (the world build runs --max-precision 5, and the cache is 12), so a test would only assert against a configuration nothing produces — but the cap is a builder option, so the code is wrong wherever it is raised and worth correcting.
🤖 Addressed by Claude Code
| }) | ||
|
|
||
| return { rows: kept, skipped: skipped } | ||
| var shadowed = await countHomeCellsShadowedByFinerRows(db, kept, placeById, batchPlaceIds, homeCellPriority) |
There was a problem hiding this comment.
When an appended place emits a coarse home cell that an existing co-centred, more populous place wins at the exact hash, kept no longer contains any row for the challenger, so the descendant scan skips it entirely. A finer persisted row can still be an invalid shadow—for example, its owner is not centred in that finer cell while the challenger is—and the within-batch nested reconciliation would give that finer cell to the challenger, but this counter reports zero. Preserve the original emitted rows for determining which home paths need scanning rather than deriving them only from exact-conflict winners.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and fixed in #12 — the detector moved there so #11 stays single-purpose. Scan paths no longer come from the conflict-resolution winners: every place with a valid home cell is scanned whether or not it kept a row, with a red-green spec whose challenger owns no row at all after the append.
🤖 Addressed by Claude Code
| var incumbent = cellCandidateFromCompactRow(existing) | ||
| if (comparePlacesForHash(candidate.place, incumbent, candidate.hash, hashCenterCache, true) < 0) { | ||
| shadowed += 1 |
There was a problem hiding this comment.
When earlier append batches have left multiple nested rows along the same centroid path, this loop independently counts every finer row that the challenger outranks. Runtime lookup uses only the longest matching prefix, so a shallower, worse-ranked owner can increment the counter even when a deeper owner correctly outranks the challenger; multiple worse-ranked rows also inflate one shadowed home cell into several. Group candidates by appended place and evaluate only its deepest surviving stored descendant so the reported condition matches the lookup result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and fixed in #12 — the detector moved there so #11 stays single-purpose. Only the deepest owner on each centroid path is judged now, and the count is per place rather than per row, with a red-green spec putting a weak precision-4 owner under a precision-5 owner that correctly outranks the challenger.
🤖 Addressed by Claude Code
dc8ba6f to
5f70570
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
1 participant
Follow-up to #9, which was merged while Codex's last review round was still open. This closes the one finding from that round that is live on real data.
Single-purpose by design: the cross-batch shadowing detector that used to be in this PR has moved to a follow-up so this branch carries only the change that alters real output. Nothing here depends on it.
The bug
normalizePlace()refuses a home-cell claim to a centroid that falls outside its own shape — the bounding-box fallback for a concave, holed, or multipart polygon can easily land in a notch or a hole. That decision lived only in memory.cellCandidateFromCompactRow()rebuilds an--appendincumbent from its storedlatitude/longitudealone, so every batch after the one that wrote the place handed the claim straight back. A challenger genuinely centred in a contested cell could then lose it to a point that is not inside the incumbent at all — the exact inversion the home-cell rule exists to prevent.This is not hypothetical. Instrumenting the gate over real Who's On First data: 105 of 18,644 places in the seven-country slice (52 of 3,354 in a ch/at/nl/cz probe) have a centroid outside their own polygon. The world database is built as 26
--appendbatches, so 25 of them see those incumbents through this path.The fix
Store the decision in a nullable
compact_places.centroid_insideand read it back on append.NULLmeans "not recorded" and keeps the pre-upgrade behaviour of trusting the coordinates, so existing databases are unchanged until they are rebuilt.ALTER TABLE ... ADD COLUMN, alongsidepopulation/area.Catalogued as schema generation 5 in
COMPATIBILITY.md, with its own permanent reader fixture inspec/reader_compatibility_spec.jspinning all three states a row can carry —1,0, and theNULLevery pre-upgrade row keeps.Validation
Red-green:
does not restore a home claim for a centroid outside its own geometry. A horseshoe whose bbox midpoint lands in its slot, and a town in the next country genuinely centred in the cell that midpoint falls in — a cell the horseshoe really does clip, so it legitimately emits it. The two are written in separate--appendbatches. Ignoring the stored flag fails withExpected 9001 to equal 9002/Expected 'US' to equal 'MX'.Full suite green: 220 specs, 0 failures.
Closes the review thread at #9 (comment).