#41: Landcover absorption drifts every class into the dominant one #56
No reviewers
Labels
No labels
claude-wip
map-report
map:is
map:se
needs-review
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
art/ismap!56
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "night/issue-41-absorption-drift"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #41
#41 Landcover absorption drifts every class into the dominant one — mechanism, fix, verification
Mechanism (measured on the live DB before any change)
Reproduced the issue's z9..z5 table on the current DB (SE/PL cover as rebuilt by #47):
km². Sweden loses 98.6% of its agricultural area and 99.4% of its artificial area
between z9 and z5, both into nature; Poland drifts the other way (agricultural +3.6%,
artificial +69%). The z9→z8 step alone eats 39 073 of the 86 405 km² of Swedish
agricultural.
Why the rollup does not save it: the z9 rollup already merges touching same-class
polygons into connected components — measured 684 residual adjacent pairs in Sweden's
42 642 z9 agricultural polygons — so 94% of the z9 agricultural polygons (40 037 of
42 642, 42 500 km²) sit individually below the z8 candidate threshold (5.98 km² at z8)
while the components they form are collectively the regions a reader can see. "Merge
adjacent same-class first and test the cluster" is therefore a no-op below z9 as
literally stated: the touching-merge has already happened. The region is the components
that sit within a patch-size of each other (sqrt(min_area_pixels) px — 4 at the working
value 16), which the rollup (by construction) does
not merge — and which absorption then eats one component at a time.
Fix shape chosen and why
Cluster-before-absorbing, with the cluster defined by distance, applied below the
coarsest rollup (
importer/generalize.go,absorbSQL): below the layer's coarsestenum_rollupszoom (z9 forcover), the area candidacy test runs on the same-classcluster — every same-class polygon within sqrt(min_area_pixels) screen pixels of each
other, 4 at the working value 16, the side of the smallest visible patch
(
ST_ClusterDBSCAN(geom, sqrt(min_area_pixels) * pixelSize, 1) OVER (PARTITION BY cover_class)— class only, deliberately not part_key) — instead of the polygon's ownarea. A one-pixel eps was measured to fail: Stockholm's built-up fragments sit across
bays and parks with gaps wider than a pixel and were eaten fragment by fragment from z8
up, where the patch-side eps chain-connects them. The width test is switched off below
the rollup — a thin sliver inside a cluster that clears the threshold is no longer a
candidate at all, where before it was; the neighbour lookup, the direction rule and the
no-neighbour drop floor are untouched; like
absorb_boundary, the floor only changeswhich polygons are candidates — a recolour, never a filter — so the coverage stays
hole-free.
Why distance rather than the issue's literal "merge adjacent": measured above — the
touching-merge is already done by the z9 rollup; the distance is what re-forms regions at
coarse zoom (a pixel is 611 m at z8, 4.9 km at z5).
Why it stops at the coarsest rollup (measured cost, not convenience):
ST_ClusterDBSCANis O(n²) in the partition size — on the z12 table (3.2 M rows) a single cell measured
4m55s (largest class partition 18 742 polygons), and the mid-class thresholds (0.09
km² at z11) are sub-feature so the per-polygon rule is already correct above z9. Below the
rollup the per-batch (part_key, class) partitions are small: 6 301 polygons through
coverage-simplify plus clustering measured 1.1 s, and the whole floor costs seconds
per step. The scoping is rollup-derived, not configured — move a rollup and the floor
follows — and z11/z10 generate byte-identical SQL, so z9 and above are unchanged by
construction and the rebuild window is z3–z8 only.
The cluster is batch-scoped: the parallel cascade splits the work into partition
batches capped at
-batch-rowspolygons, each running its ownabsorbSQLwith its owntemp table, so the DBSCAN only ever sees one batch — a region spanning a batch boundary
is still split, one level up from the per-cell failure. All measurements below assume
-batch-rows 10000.Rejected alternatives, with numbers:
(part_key, cover_class) instead of clustering. Zero cost, and it recovers more area at
z8 (86 268 km² of SE agricultural vs 74 145 for the cluster) — but it protects every
member of a class in a cell whose total clears the threshold, including isolated
sub-pixel specks, and it splits cross-cell regions at cell boundaries that the cluster
re-joins. The cluster keeps the de-speckling (an isolated speck's cluster is itself and
still absorbs) and costs seconds below the rollup, so the only reason to take the floor
would be the z12 cost — which the rollup scoping already removes.
it is a candidate; the ratchet is about candidacy (94% of z9 agricultural is
sub-threshold at z8), so this does not fix the measured drift.
Verification
make build;go test ./...(importer + mapsrv) — pass, including the newTestAbsorbClusterFloor(pins the_cl_acandidate test and the DBSCAN partitionbelow the rollup, the per-polygon rule above it and for no-rollup layers, and the
unchanged drop floor).
Regeneralized
coverz3–z8 from the live z9 for SE and PL (z9+ unchanged byconstruction — see above). Final combined-build numbers (
-regeneralize 3-8 -jobs 6 -batch-rows 10000; the branch's cluster floor AND the #47 cross-boundary floor, whosecommits this branch carries — see the supersede note below — the build the DB had not
had yet):
coverkm²coverkm²The z9→z8 step now eats 7 119 km² of Swedish agricultural (86 405 → 79 286) instead
of 39 073, and the z9→z5 picture is: SE agricultural −98.6% → −53.2% (86 405 →
40 469 km²), SE artificial −99.4% → −93.1% (10 083 → 695 km²), PL agricultural
+3.6% → +2.7% (508 304 km² at z5), PL artificial +69% → −4.9% (25 988 km² at
z5). The #47 floor shows in the water line: SE z5 water holds at 568 385 km², within
0.3% of the z9 seed (570 056), where the #47-only build had retreated 5%.
Coverage hole-free: the check that actually constrains this failure mode is the
per-zoom totals, and they hold — SE 2 562 190 km² (z9) → 2 561 943 (z8) → 2 570 330
(z5), PL 906 000 (z9) → 904 820 (z5), all within 0.3%. (The earlier "0 NULL cover_class
rows" check tests classification, not hole-freeness: a polygon falling out of both
halves of the small/large split disappears entirely rather than appearing with a NULL
class.)
Decoded tile (mvt-decode.py + attribute walk):
zxy_map(6,34,20)(southern Skåne)and
zxy_map(5,17,10)carry 4 222 and 17 950coverfeatures classifiedagricultural— farmland is the dominant class in the served tiles.Real-browser CDP check (mapsrv :18080, headless Chrome,
#5/55.6/13.0): 35 205rendered
cover_agriculturalfeatures vs 32 145 nature — Skåne reads as farmland,not uniform forest. No console errors.
#47 cross-reference and supersede
#47 (water eats coastlines, PR #55) floored cross-boundary inheritance at one screen
pixel — the coastline can only move by sub-pixel amounts per step. This fix floors the
inland ratchet: sub-threshold polygons are tested as same-class regions. They are
distinct mechanisms (boundary floor vs region cluster), neither retires the other; #47
stays open on its own merits and this PR's target is #41.
This branch carries the #47 fix in full — the absorb_boundary cross-boundary floor
(
78dad2f), its mechanism documentation (4c3bcd4), and the combinedcluster-floor-does-not-disable-#47 test (
8ad8517). Merging this PR alone would ship the#47 fix while #47 stays open and PR #55 becomes an empty PR. Since #55 is not merged and
this branch contains its content, this PR supersedes #55: the maintainer may retire
PR #55 in this one's favour at merge time. The incremental review surface for the
#41-specific work is
git diff origin/night/issue-47-water-coastline..HEAD— the 153lines of
absorbSQLactually about #41. Note that the #47 branch has since been rebasedand extended (its current tip carries a guard-covers-width doc note,
50ccd57); thatnote is incorporated into this branch's
docs/generalization.md, so retiring #55 losesnothing from its current tip. The verification numbers below combine both fixes and hold
only if the combined code (this branch) is what lands.
Left undone
coverwas not rebuilt (FI is still on the pre-#47 logic; rebuild onmerge — it has neither the cluster floor nor the cross-boundary floor).
Review — #56 (landcover absorption drift, issue #41)
Builds clean,
go vetandgo testpass (the only failure isTestDumpGlyphPNG, whichcannot write
/tmp/glyph_g.pngin this sandbox and fails identically on every branchincluding the docs-only one).
The mechanism work is the best part of this PR. Measuring 684 residual adjacent pairs in
42 642 z9 agricultural polygons to show that the z9 rollup has already done the
touching-merge — and therefore that the issue's literal "merge adjacent same-class first"
is a no-op below z9 — is the finding that turns the whole thing from a guess into a design.
Reaching for distance instead, and deriving the scope from
coarsestRollupZoomrather thana new config knob, is the right shape. The two-UPDATE materialisation with the note that
the planner re-evaluates a window function referenced by a second window is a good catch and
exactly the kind of thing that belongs in a comment.
Four things before merge.
1. This branch contains PR #55 in full
The body describes the #47 cross-boundary floor as "cherry-picked" into the verification
build, which reads as a local convenience. It is not — the branch carries those commits:
git diff main...for this branch therefore includes all of #55. So merging #56 on its ownwould silently ship the #47 fix while issue #47 stays open and #55 becomes an empty PR —
awkward given the body explicitly says "this change leaves #47 open". Either rebase onto
mainafter #55 lands, or say plainly that #56 supersedes #55 and retire #55 in its favour.The incremental review surface here is
git diff origin/night/issue-47-water-coastline..,which is the 153 lines of
absorbSQLthat are actually about #41.It also means the verification numbers cannot separate the two fixes, which the body is
honest about but which is worth restating: if #55 does not merge, these tables do not hold.
2. Width-only config generates SQL referencing a column that is never created
clusteris lowered tofalseat generalize.go:889 when the layer has no class columns ormin_area_pixels == 0— butareaTest,candPredandnotCandPredwere already builtfrom the original
clusterat lines 835–850. So a width-only cover layer below thecoarsest rollup emits
_cl_apredicates whileclusterSQLstays empty and the column isnever added. Reproduced with a throwaway test on this branch:
That is
column "_cl_a" does not existat runtime. No current config reaches it — everycoverlayer setsmin_area_pixels— but width-only is an explicitly supported shape, andabsorbSQL's own comment documents it ("a width-only config leaves min_area_pixels unset…absorption is on because min_width_pixels turned it on"). Moving the
if len(classCols) == 0 || l.MinAreaPixels == 0 { cluster = false }block above theareaTestcomputation fixes it. Worth a test case, since nothing currently exercises thepath.
3. Three places still document the eps that was measured not to work
633882achanged the cluster distance from one pixel tosqrt(min_area_pixels) * px(4 px at the working value) and changed the partition from
part_key, classto class only.absorbSQL's doc comment andTestAbsorbClusterFloor's assertions were updated. These werenot:
conf/common.yaml— "every same-class polygon within one screen pixel"docs/generalization.md:142— "within one screen pixel(
ST_ClusterDBSCAN(geom, pixelSize, 1) OVER (PARTITION BY cover_class))"This matters more than ordinary doc drift: the eps is the single most consequential constant
in the change, and the commit that changed it says 1 px "ate Stockholm fragment by fragment
from z8 up". All three currently describe the version that was measured to fail.
Minor, same family:
TestAbsorbClusterFloor's opening docstring says "_cl_a,ST_ClusterDBSCAN over (partition, class) at one screen pixel" while the assertions below it
correctly check class-only and the sqrt eps.
4. The cluster is per-batch, not global — so output depends on
-batch-rowsgeneralizeZParallelsplits the work withpartitionBatches, capped atbatchRowspolygons, and each batch runs its own
absorbSQLwith its own_cover_absorb. The DBSCANtherefore only sees one batch. "The cluster deliberately crosses the processing partition
(it does not partition by part_key)" is true within a batch and not across batches, so a
region spanning a batch boundary is still split — the same failure the comment says
per-cell clustering caused, one level up.
Stockholm survived at the
-batch-rows 10000used for verification, but that makes arendering outcome depend on a performance flag, which is the kind of thing that will be very
confusing the first time someone reruns with the 50 000 default and gets different tiles.
At minimum the comment and
docs/generalization.mdshould say the cluster isbatch-scoped; better would be a note on what batch size the measured behaviour assumes.
On the hole-free check
"0 NULL cover_class rows at z3–z8" tests classification, not hole-freeness — a polygon
falling out of both halves of the small/large split disappears entirely rather than
appearing with a NULL class, so that check would not see it. The published km² tables do
cover it, and they pass: SE totals 2 562 190 (z9) → 2 561 943 (z8) → 2 570 330 (z5), PL
906 000 (z9) → 904 820 (z5), all within 0.3%. Worth citing that as the hole check
instead, since it is the one that actually constrains the failure mode.
Related:
_cl_ahas noCOALESCEguard, where_wgot one precisely because a NULL therewould drop a row out of both halves. With
minpoints=1every row should get a cluster id soit should not be reachable, and the area conservation above says it is not happening — but
the asymmetry with
_wis worth a line of comment either way.Smaller notes
min_width_pixelsis switched off entirely below the rollup (if minWidth > 0 && !cluster)."Subsumed by the cluster test" is not quite the right word: a thin sliver inside a cluster
that clears the threshold is no longer a candidate at all, where before it was. Combined
with #55 — which already lets width candidates keep their class across the boundary — that
knob now does substantially less than its name suggests, and does different things above
and below z9. Both changes are defensible; the combination deserves one sentence somewhere
so the next person to tune it is not surprised.
clusterEps² == minAreaexactly, so "the side of a square of the threshold area" isself-consistent and scales with the config as claimed. Nice.
a large improvement on 98.6%, but it is not obviously the end of #41 — worth deciding
whether closing the issue here is right, or whether the residual deserves its own follow-up.
69490e7533to6d6c931975Review addressed on
night/issue-41-absorption-drift— rebased onto current main (was 5 behind) and pushed as69490e7…6d6c931with two new commits. Point by point:1. Supersede / branch contains PR #55 — decided: #56 supersedes #55, and the body now says so. We never merge or retire PRs ourselves, so rather than rebase onto a #55 that has not landed, the body states it plainly: this branch carries the #47 fix in full —
78dad2f(absorb_boundary cross-boundary floor),4c3bcd4(its mechanism docs),8ad8517(the combined cluster-floor-does-not-disable-#47 test); those are the post-rebase SHAs of the review's65d57a9/79155c0/08d94f6. The maintainer may retire PR #55 in this one's favour at merge time; the #41-only review surface isgit diff origin/night/issue-47-water-coastline..HEAD. One thing changed since the review: the #47 branch was itself rebased and extended — its current tip carries50ccd57, the guard-covers-width doc note from the #55 review round. That note is now incorporated into this branch'sdocs/generalization.md, so retiring #55 loses nothing from its current tip.2. The width-only
_cl_abug — fixed, with a test. Thecluster = falselowering (and theclassColscomputation it depends on) now runs before the candidate test is built, so a width-only layer below the coarsest rollup can no longer emit_cl_apredicates whileclusterSQLstays empty. Reproduced first with a newTestAbsorbClusterFloorWidthOnlyon the pre-fix code:WHERE _cl_a <= 0.00andWHERE _cl_a > 0.00with no_cl_adefined — your repro, verbatim. It passes after the fix, and additionally asserts the width path itself is intact (ADD COLUMN _w float8,COALESCE(_w, 'Infinity'::float8)still in the candidate test).make build,go test ./...(importer + mapsrv) andgo vetall green. Code-only change: the live DB is untouched — it takes effect on the next import.3. The three stale eps sites — corrected.
conf/common.yaml,docs/generalization.md:142and the PR body's "Fix shape chosen and why" now all say the cluster is every same-class polygon withinsqrt(min_area_pixels) * pixelSize(4 px at the working value 16) over the class column only — the version that was measured to work, not the 1 px / (part_key, class) version that lost Stockholm.TestAbsorbClusterFloor's opening docstring is fixed the same way; its assertions already checked class-only and the sqrt eps.4. Cluster is batch-scoped — documented, not papered over. Agreed:
generalizeZParallelcaps batches atbatchRowspolygons and each batch runs its ownabsorbSQLwith its own_cover_absorb, so the DBSCAN only ever sees one batch. The doc comment,docs/generalization.mdand the PR body now say so explicitly, and note the measured behaviour assumes-batch-rows 10000. "Crosses the processing partition" is qualified to within a batch; a region spanning a batch boundary is still split — a known limitation, not a silent one. A rendering outcome depending on a performance flag is exactly the thing to flag; it is now flagged in all three places.5. Smaller notes.
_cl_ahas no COALESCE guard where_wgot one: noted in a comment next toclusterSQL. Withminpoints=1every row gets a cluster id, so the sum is never NULL — and were it reachable, a NULL would drop the row out of both halves exactly like a NULL_wdid. No behavioural change.min_width_pixelsbelow the rollup: reworded everywhere from "subsumed" to "switched off" — a thin sliver inside a cluster that clears the threshold is no longer a candidate at all, where before it was. Combined with the #47 floor (which also lets width candidates keep their class across the line), the knob does less below the rollup than its name suggests; it still runs above the rollup and for no-rollup layers. One sentence each in the code comment, the docs and the PR body.FI cover remains un-rebuilt (noted in "Left undone" — it gets both floors on merge).