art/ismap
1
0
Fork 0
Code Issues 18 Pull requests 1 Projects Releases Packages Wiki Activity Actions

waterways: split centreline junctions on linestring interiors, stitch adjacent water polygons #67

Open
art wants to merge 3 commits from night/issue-62-water-flow into main
pull from: night/issue-62-water-flow
merge into: art:main
Conversation 2 Commits 3 Files changed 2 +148 -18
art commented 2026-08-06 13:02:02 +00:00
Owner
Copy link

#62 waterways: mid-point junction to river polygon missed by the Strahler bridge — fixed

Fixes the inverted water-flow importance at 65.55624/-17.69235 (z20, map=is).

Reporter's hypothesis: CONFIRMED. The Strahler network graph is built from
node keys that exist only at linestring endpoints, so a junction on the
interior of a linestring is invisible. The reported segment touches the river
polygon NW of it at interior vertex 5 (exactly the polygon's tip vertex), while
both endpoints are 267 m / 1006 m from the polygon — far outside the 30 m
bridge tolerance. The endpoint-only bridge never joined the segment to the
polygon, its drainage rooted elsewhere, and the orders inverted: the segment
was importance 1 while everything south of it was 2.

Root cause (two gaps in the same place):

  1. Interior junctions are never split: _ww_seg built one edge per linestring
    with node keys only at its ends.
  2. Adjacent water polygons never connect: a river digitized as a chain of
    touching polygons (here 7 river polygons running north to the coast) is one
    water body, but every polygon was its own super-node and no stream endpoint
    bridges two adjacent ones — so the chain, and anything that drains into it,
    rooted nowhere. Even with the split, the segment's component had no ocean
    node and the inversion persisted.

Fix (importer/strahler.go):

  1. Split centrelines at every interior junction point (boundary crossings and
    touches of bridging water polygons, including mid-edge ones, plus other
    segments' endpoints landing on the interior) before building the edge list;
    each piece becomes its own edge, so the junction node exists and inherits the
    polygon's pid.
  2. Stitch adjacent bridging polygons with virtual edges (shared LINESTRING
    boundary only — a point-touch is not a connection), like the ocean edges.

Verification (live DB, reimported -layer waterways conf/ismap.yaml):

  • Reported row (10859,15): importance 1 → 4; everything south of it is 4
    — no longer higher. The z14 tile decodes with importance 4 on the reported
    watercourse; real-browser check at z19.58 renders four waterways around the
    junction, all importance 4.
  • The 7-polygon river chain is now one ocean-rooted component (6 ocean nodes)
    instead of a dead-end super-node.
  • Iceland histogram 50,652/17,316/9,036/3,428/1,473 across orders 1–5,
    Horton-like, zero NULLs; per-zoom partitions cascade sanely.
  • Shared code (SE/FI run the same stage): row counts unchanged (SE 838,973,
    FI 626,990). Replications reproduce each country's live histogram exactly
    under the old rules (tool validated) and remain monotone Horton ladders under
    the new rules; the order-5 gains (SE 6,627→14,657, FI 479→3,673) come from
    the polygon-chain stitching (22,099 / 6,964 adjacent pairs), the same class
    of connection the old code missed. PL does not use the strahler stage.
#62 waterways: mid-point junction to river polygon missed by the Strahler bridge — fixed Fixes the inverted water-flow importance at 65.55624/-17.69235 (z20, map=is). **Reporter's hypothesis: CONFIRMED.** The Strahler network graph is built from node keys that exist only at linestring endpoints, so a junction on the *interior* of a linestring is invisible. The reported segment touches the river polygon NW of it at interior vertex 5 (exactly the polygon's tip vertex), while both endpoints are 267 m / 1006 m from the polygon — far outside the 30 m bridge tolerance. The endpoint-only bridge never joined the segment to the polygon, its drainage rooted elsewhere, and the orders inverted: the segment was importance 1 while everything south of it was 2. **Root cause (two gaps in the same place):** 1. Interior junctions are never split: `_ww_seg` built one edge per linestring with node keys only at its ends. 2. Adjacent water polygons never connect: a river digitized as a chain of touching polygons (here 7 river polygons running north to the coast) is one water body, but every polygon was its own super-node and no stream endpoint bridges two adjacent ones — so the chain, and anything that drains into it, rooted nowhere. Even with the split, the segment's component had no ocean node and the inversion persisted. **Fix (importer/strahler.go):** 1. Split centrelines at every interior junction point (boundary crossings and touches of bridging water polygons, including mid-edge ones, plus other segments' endpoints landing on the interior) before building the edge list; each piece becomes its own edge, so the junction node exists and inherits the polygon's pid. 2. Stitch adjacent bridging polygons with virtual edges (shared LINESTRING boundary only — a point-touch is not a connection), like the ocean edges. **Verification (live DB, reimported `-layer waterways conf/ismap.yaml`):** - Reported row `(10859,15)`: importance **1 → 4**; everything south of it is 4 — no longer higher. The z14 tile decodes with importance 4 on the reported watercourse; real-browser check at z19.58 renders four waterways around the junction, all importance 4. - The 7-polygon river chain is now one ocean-rooted component (6 ocean nodes) instead of a dead-end super-node. - Iceland histogram 50,652/17,316/9,036/3,428/1,473 across orders 1–5, Horton-like, zero NULLs; per-zoom partitions cascade sanely. - Shared code (SE/FI run the same stage): row counts unchanged (SE 838,973, FI 626,990). Replications reproduce each country's live histogram exactly under the old rules (tool validated) and remain monotone Horton ladders under the new rules; the order-5 gains (SE 6,627→14,657, FI 479→3,673) come from the polygon-chain stitching (22,099 / 6,964 adjacent pairs), the same class of connection the old code missed. PL does not use the strahler stage.
The Strahler network graph is built from node keys that only exist at linestring
endpoints, so a junction on the interior of a linestring was invisible to it.
Issue #62: a channel drains into a wide river polygon at a mid-point vertex of
its linestring (both endpoints 267 m / 1006 m from the polygon, far outside the
30 m bridge tolerance), so the endpoint-only bridge never joined it to the
polygon, its drainage rooted elsewhere, and the orders inverted — the reported
segment showed importance 1 while everything south of it showed higher.

Two fixes in importer/strahler.go:

1. Split centrelines at interior junction points before building the edge list.
   Any point where a line meets a bridging water polygon (boundary crossings and
   touches, including mid-edge ones) or where another segment's endpoint lands
   on its interior becomes a blade; each piece becomes its own edge, so the
   junction node exists and can inherit the polygon's pid. Also fixes tributary
   endpoints that hang off another stream's interior.

2. Stitch adjacent bridging polygons. A river digitized as a chain of touching
   polygons is one water body, but every polygon was its own super-node and no
   stream endpoint bridges two adjacent ones, so the chain — and anything that
   drains into it — rooted nowhere. Polygons that share an edge (LINESTRING
   intersection, not a point-touch: water cannot flow through a corner) and both
   carry a stream node are connected with virtual edges, like the ocean edges.

Reported location (65.55624/-17.69235) after reimport: the segment is
importance 4, everything south is 4 — no longer higher. The z14 tile decodes
with importance 4 on the reported watercourse, and a real-browser check at
z19.58 renders four waterways at importance 4 around the junction. The polygon
chain (7 touching river polygons to the coast) is now one ocean-rooted
component (6 ocean nodes) instead of a dead-end super-node.

Iceland histogram after reimport (81,905 rows, unchanged): 50,652 / 17,316 /
9,036 / 3,428 / 1,473 across orders 1-5 (clamped at 5) — Horton-like, no NULLs.
Per-zoom partitions cascade sanely (z14=z13=81,905; z12=52,862; z11=z10=13,135;
z9=z8=4,961; z5=511).

Shared code: the same stage runs for SE and FI. Row counts are unchanged
(SE 838,973, FI 626,990). Both countries' replications of the new graph
reproduce their live histograms exactly under the old rules, validating the
tool; under the new rules SE goes 496,289/204,866/96,356/34,835/6,627 →
507,426/204,727/80,794/31,369/14,657 and FI 387,145/177,477/51,584/10,305/479
→ 398,015/165,214/48,532/11,556/3,673: still monotone-decreasing Horton
ladders, with the order-5 gain coming from the polygon-chain stitching (22,099
adjacent pairs in SE, 6,964 in FI) — the same class of connection the old code
missed. PL does not use the strahler stage.

docs/schema.md documents the two new stitches.

Co-Authored-By: Hermes Agent <noreply@nousresearch.com>
art-bot commented 2026-08-06 17:10:19 +00:00
Collaborator
Copy link

Review

The root-cause analysis is correct and the fix is the right shape. Verified Iceland's claimed histogram exactly (50,652 / 17,316 / 9,036 / 3,428 / 1,473, zero NULLs), and confirmed PL genuinely has no strahler stage. Builds, vet and tests pass. The _ww_ends → _ww_junc cleanup fix in 681fbcb is correct.

Three things worth addressing.

1. The adjacency filter silently drops real connections — importer/strahler.go:169

GeometryType(ST_Intersection(a.geom,b.geom)) IN ('LINESTRING','MULTILINESTRING') excludes two genuine cases:

  • GEOMETRYCOLLECTION — polygons sharing an edge and touching at a stray point elsewhere. Measured: IS 2, SE 12, FI 5. I dumped both Iceland cases; each is a real shared LINESTRING plus point-touches, e.g.
    GEOMETRYCOLLECTION(LINESTRING(-1810523.6 9578196.6,-1810408.0 9578191.7),POINT(…),POINT(…))
  • POLYGON — genuinely overlapping polygons, unambiguously one water body. SE has 74 such pairs.

Full breakdown of linear/areal intersections between bridging polygons:

type IS SE FI
LINESTRING 366 45,978 12,810
MULTILINESTRING 289 3,033 7,605
POINT 25 1,244 157
MULTIPOINT 1 36 —
POLYGON (dropped) — 74 —
GEOMETRYCOLLECTION (dropped) 2 12 5

ST_Dimension(ST_Intersection(a.geom,b.geom)) >= 1 is the predicate you want — verified it returns 1 for GC-with-a-line, 2 for polygons, 0 for POINT/MULTIPOINT, so it preserves the point-touch exclusion the comment argues for while admitting the rest.

The count looks small, but impact isn't proportional: one missing link disconnects an entire chain, which is precisely the failure #62 was about.

2. The "both polygons must carry a stream node" filter breaks pass-through chains — strahler.go:170-171

The comment argues "a polygon with none cannot be a super-node, so stitching it is meaningless." That's the wrong test for a middle link. In A(used) — B(unused) — C(used), the Go padj loop creates P_B in adj on its own, so including B would correctly carry drainage A→C. Excluding it drops both A–B and B–C and breaks the chain.

Measured on Iceland: of 657 linear adjacencies only 435 survive the filter.

a_used b_used pairs
t t 435
t f 130
f t 51
f f 41

Dropping the filter links 37 more used polygons into chains and grows the largest chain from 34 to 37 polygons:

both-used only (PR rule)     edges= 435  used-polys in a chain= 566  chains=135  largest=34
all linear adjacencies       edges= 657  used-polys in a chain= 603  chains=137  largest=37

Modest, but it's the same class of bug you're fixing.

3. SE and FI were never reimported

The live DB still holds the pre-change SE 6,627 / FI 479 order-5 counts. The claimed SE 14,657 / FI 3,673 come from the replication model, not from running the stage. FI's top rung moving 479 → 3,673 is a 7.7× jump — a visible line-width change across the whole country's river network. Iceland got real live-DB verification; the two countries with the largest deltas got a model of one. AGENTS.md asks for the former.

Minor

  • CREATE INDEX ON _ww_padj (pa) is never used — the only read is a full-scan SELECT pa, pb … ORDER BY pa, pb.
  • Tolerance inconsistency: the endpoint-on-interior split uses exact ST_Intersects, while endpoint-to-endpoint joins tolerate snap_meters (default 1 m). An endpoint 0.5 m off another line's interior still hangs off nothing.

Disposition: wants the ST_Dimension fix and an SE/FI reimport before merge; the pass-through filter is a judgement call that could reasonably be a follow-up.

The one mapsrv test failure (glyph_dump_test.go:40: open /tmp/glyph_g.png: permission denied) is environmental and reproduces on main — not this PR.

## Review The root-cause analysis is correct and the fix is the right shape. Verified Iceland's claimed histogram exactly (50,652 / 17,316 / 9,036 / 3,428 / 1,473, zero NULLs), and confirmed PL genuinely has no strahler stage. Builds, vet and tests pass. The `_ww_ends` → `_ww_junc` cleanup fix in 681fbcb is correct. Three things worth addressing. ### 1. The adjacency filter silently drops real connections — `importer/strahler.go:169` `GeometryType(ST_Intersection(a.geom,b.geom)) IN ('LINESTRING','MULTILINESTRING')` excludes two genuine cases: - **GEOMETRYCOLLECTION** — polygons sharing an edge *and* touching at a stray point elsewhere. Measured: IS 2, SE 12, FI 5. I dumped both Iceland cases; each is a real shared `LINESTRING` plus point-touches, e.g. `GEOMETRYCOLLECTION(LINESTRING(-1810523.6 9578196.6,-1810408.0 9578191.7),POINT(…),POINT(…))` - **POLYGON** — genuinely *overlapping* polygons, unambiguously one water body. SE has 74 such pairs. Full breakdown of linear/areal intersections between bridging polygons: | type | IS | SE | FI | |---|---|---|---| | LINESTRING | 366 | 45,978 | 12,810 | | MULTILINESTRING | 289 | 3,033 | 7,605 | | POINT | 25 | 1,244 | 157 | | MULTIPOINT | 1 | 36 | — | | **POLYGON (dropped)** | — | **74** | — | | **GEOMETRYCOLLECTION (dropped)** | **2** | **12** | **5** | `ST_Dimension(ST_Intersection(a.geom,b.geom)) >= 1` is the predicate you want — verified it returns 1 for GC-with-a-line, 2 for polygons, 0 for POINT/MULTIPOINT, so it preserves the point-touch exclusion the comment argues for while admitting the rest. The count looks small, but impact isn't proportional: one missing link disconnects an entire chain, which is precisely the failure #62 was about. ### 2. The "both polygons must carry a stream node" filter breaks pass-through chains — `strahler.go:170-171` The comment argues "a polygon with none cannot be a super-node, so stitching it is meaningless." That's the wrong test for a *middle* link. In A(used) — B(unused) — C(used), the Go padj loop creates `P_B` in `adj` on its own, so including B would correctly carry drainage A→C. Excluding it drops both A–B and B–C and breaks the chain. Measured on Iceland: of 657 linear adjacencies only **435** survive the filter. | a_used | b_used | pairs | |---|---|---| | t | t | 435 | | t | f | 130 | | f | t | 51 | | f | f | 41 | Dropping the filter links 37 more used polygons into chains and grows the largest chain from 34 to 37 polygons: ``` both-used only (PR rule) edges= 435 used-polys in a chain= 566 chains=135 largest=34 all linear adjacencies edges= 657 used-polys in a chain= 603 chains=137 largest=37 ``` Modest, but it's the same class of bug you're fixing. ### 3. SE and FI were never reimported The live DB still holds the pre-change SE 6,627 / FI 479 order-5 counts. The claimed SE 14,657 / FI 3,673 come from the replication model, not from running the stage. FI's top rung moving 479 → 3,673 is a **7.7× jump** — a visible line-width change across the whole country's river network. Iceland got real live-DB verification; the two countries with the largest deltas got a model of one. AGENTS.md asks for the former. ### Minor - `CREATE INDEX ON _ww_padj (pa)` is never used — the only read is a full-scan `SELECT pa, pb … ORDER BY pa, pb`. - Tolerance inconsistency: the endpoint-on-interior split uses exact `ST_Intersects`, while endpoint-to-endpoint joins tolerate `snap_meters` (default 1 m). An endpoint 0.5 m off another line's interior still hangs off nothing. --- **Disposition:** wants the `ST_Dimension` fix and an SE/FI reimport before merge; the pass-through filter is a judgement call that could reasonably be a follow-up. The one mapsrv test failure (`glyph_dump_test.go:40: open /tmp/glyph_g.png: permission denied`) is environmental and reproduces on `main` — not this PR.
The polygon-adjacency predicate tested GeometryType against
('LINESTRING','MULTILINESTRING'), which silently dropped two kinds of real
connection:

  - a pair sharing an edge *and* touching at a stray point elsewhere
    intersects in a GEOMETRYCOLLECTION;
  - a pair that genuinely overlaps intersects in a POLYGON.

Both are one water body. Measured against the live water tables, the test
dropped 2 pairs on Iceland, 5 on Finland and 86 on Sweden (74 of Sweden's
being overlaps). The counts are small but the impact is not proportional:
one missing link disconnects a whole chain, which is the failure this issue
was filed for.

ST_Dimension(...) >= 1 is the predicate that was meant. It keeps the
point-touch exclusion the comment argues for — water cannot flow through a
corner, and POINT/MULTIPOINT are dimension 0 — while admitting the shared
edge, the overlap and the collection that contains a line.

Verified on Iceland (the only country whose source data is on this box,
reimported -layer waterways conf/ismap.yaml): orders 1-5 are
50 656 / 17 312 / 9 056 / 3 405 / 1 476, a monotone Horton ladder with zero
NULLs, the per-zoom cascade is sane (81 905 / 52 862 / 13 132 / 4 960 at
z14/12/10/8), and the watercourse reported at 65.55624/-17.69235 still
carries importance 4 — the inversion this issue was filed for stays fixed.

The other half of the predicate — that both polygons must carry a stream
node, which drops the pass-through link of a chain — is issue #70. It is
left alone here because lifting it grows the adjacency by half again on
Iceland and nearly triples it on Finland, and so needs its own verified
import per country rather than a replication.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
art-bot commented 2026-08-06 19:28:12 +00:00
Collaborator
Copy link

Review items addressed — 6244b06

1. ST_Dimension fix — done. The adjacency predicate now tests ST_Dimension(ST_Intersection(a.geom,b.geom)) >= 1 instead of GeometryType(...) IN ('LINESTRING','MULTILINESTRING'). Point-touches stay excluded (dimension 0); the shared-edge-plus-stray-point GEOMETRYCOLLECTION and the genuine overlap are now admitted.

Verified on Iceland (reimported -layer waterways conf/ismap.yaml):

orders 1–5 NULLs
before (this PR) 50 652 / 17 316 / 9 036 / 3 428 / 1 473 0
after 50 656 / 17 312 / 9 056 / 3 405 / 1 476 0

Monotone Horton ladder, per-zoom cascade sane (81 905 / 52 862 / 13 132 / 4 960 at z14/12/10/8), and the watercourse at 65.55624/-17.69235 still carries importance 4 — #62's inversion stays fixed.

2. Pass-through polygons — split out as #70, not fixed here. On measuring it, lifting the both-ends-used filter is a much bigger change than I implied when I raised it:

old rule with ST_Dimension + pass-through
IS 434 436 657
FI 7 032 7 037 20 420
SE 22 783 22 869 49 097

Half again on Iceland, nearly triple on Finland. That deserves its own verified import rather than riding along here. #70 carries the full measurements and the done-when.

(The old-rule column lands within 3% of the 22 099 SE / 6 964 FI this PR reported, which is the check that the replication is faithful.)

3. SE/FI reimport — still open, and I could not close it. Only Iceland's source data is on the dev box: data/se_topo10/hydro_sverige.gpkg is absent (only kommunikation_sverige.gpkg is local) and data/fi/ is empty. -layer waterways conf/semap.yaml therefore kicks off a full ~11 GB Geotorget delivery, which I stopped. This needs running where the data already lives. It applies to the PR's own order-5 shifts (FI 479 → 3 673 by replication) as much as to my change.

Minor: dropped the unused CREATE INDEX ON _ww_padj (pa) — the only read is a full-scan SELECT pa, pb … ORDER BY pa, pb, which a btree on pa alone cannot serve. The tolerance inconsistency I noted (exact ST_Intersects for endpoint-on-interior vs snap_meters elsewhere) is left as-is; it is a design question, not a defect.

go build, go vet and go test pass on importer. Worth noting go vet earns its keep here — prepSQL builds the SQL through fmt.Sprintf, so a literal % in a comment inside that string is a format verb; vet caught one I introduced.

## Review items addressed — 6244b06 **1. `ST_Dimension` fix — done.** The adjacency predicate now tests `ST_Dimension(ST_Intersection(a.geom,b.geom)) >= 1` instead of `GeometryType(...) IN ('LINESTRING','MULTILINESTRING')`. Point-touches stay excluded (dimension 0); the shared-edge-plus-stray-point GEOMETRYCOLLECTION and the genuine overlap are now admitted. **Verified on Iceland** (reimported `-layer waterways conf/ismap.yaml`): | | orders 1–5 | NULLs | |---|---|---| | before (this PR) | 50 652 / 17 316 / 9 036 / 3 428 / 1 473 | 0 | | after | 50 656 / 17 312 / 9 056 / 3 405 / 1 476 | 0 | Monotone Horton ladder, per-zoom cascade sane (81 905 / 52 862 / 13 132 / 4 960 at z14/12/10/8), and the watercourse at 65.55624/-17.69235 still carries importance 4 — #62's inversion stays fixed. **2. Pass-through polygons — split out as #70, not fixed here.** On measuring it, lifting the both-ends-used filter is a much bigger change than I implied when I raised it: | | old rule | with `ST_Dimension` | + pass-through | |---|---|---|---| | IS | 434 | 436 | **657** | | FI | 7 032 | 7 037 | **20 420** | | SE | 22 783 | 22 869 | **49 097** | Half again on Iceland, nearly triple on Finland. That deserves its own verified import rather than riding along here. #70 carries the full measurements and the done-when. (The old-rule column lands within 3% of the 22 099 SE / 6 964 FI this PR reported, which is the check that the replication is faithful.) **3. SE/FI reimport — still open, and I could not close it.** Only Iceland's source data is on the dev box: `data/se_topo10/hydro_sverige.gpkg` is absent (only `kommunikation_sverige.gpkg` is local) and `data/fi/` is empty. `-layer waterways conf/semap.yaml` therefore kicks off a full ~11 GB Geotorget delivery, which I stopped. This needs running where the data already lives. It applies to the PR's own order-5 shifts (FI 479 → 3 673 by replication) as much as to my change. **Minor:** dropped the unused `CREATE INDEX ON _ww_padj (pa)` — the only read is a full-scan `SELECT pa, pb … ORDER BY pa, pb`, which a btree on `pa` alone cannot serve. The tolerance inconsistency I noted (exact `ST_Intersects` for endpoint-on-interior vs `snap_meters` elsewhere) is left as-is; it is a design question, not a defect. `go build`, `go vet` and `go test` pass on `importer`. Worth noting `go vet` earns its keep here — `prepSQL` builds the SQL through `fmt.Sprintf`, so a literal `%` in a comment inside that string is a format verb; vet caught one I introduced.
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin night/issue-62-water-flow:night/issue-62-water-flow
git switch night/issue-62-water-flow

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff night/issue-62-water-flow
git switch night/issue-62-water-flow
git rebase main
git switch main
git merge --ff-only night/issue-62-water-flow
git switch night/issue-62-water-flow
git rebase main
git switch main
git merge --no-ff night/issue-62-water-flow
git switch main
git merge --squash night/issue-62-water-flow
git switch main
git merge --ff-only night/issue-62-water-flow
git switch main
git merge night/issue-62-water-flow
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
claude-wip
night-shift

  
map-report
map data feedback (mapsrv)

  
map:is
map data feedback (mapsrv)

  
map:se
map data feedback (mapsrv)

  
needs-review
night-shift

No labels
claude-wip
map-report
map:is
map:se
needs-review
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
art/ismap!67
No description provided.