art/ismap

Fork 0

#70: node-less bridging polygons stay in the adjacency, so pass-through chains conduct #96

Closed

art wants to merge 4 commits from issue-70-strahler-pass-through into main

art commented

2026-08-10 13:40:14 +00:00

Owner

Closes #70

#70 strahler: node-less bridging polygons stay in the adjacency — pass-through chains now conduct

Lifts the both-used filter from _ww_padj in importer/strahler.go: the two
AND ... pid IN (SELECT pid FROM _ww_node WHERE pid > 0) clauses are gone, so a
bridging polygon with no stream node is no longer dropped from the adjacency. It
cannot originate drainage — nothing roots there — but it conducts, and
computeStrahlerOrders materialises its P<pid> super-node from the padj
adjacency itself, so an A–B–C chain stays connected when B is node-less.

Why

The old comment argued a node-less polygon cannot be a super-node, "so stitching
it is meaningless". That is the right test for a polygon at the end of a chain
and the wrong one for a polygon in the middle: A—B—C with B node-less lost both
A—B and B—C, breaking the chain so A's drainage never reached C — the same
failure mode #62 was filed for. The issue's measurements: adjacent pairs
434→657 (IS), 7,032→20,420 (FI), 22,783→49,097 (SE). I re-verified the pair
counts against the live tables (FI = 20,420 directly).

Verification — IS, SE and FI each reimported `-layer waterways` against real source data

importer/importer -layer waterways -jobs 4 -batch-rows 10000 conf/{ismap,semap,fimap}.yaml.
No new downloads; source data on disk. Live swaps all succeeded.

Per-order histogram (map_waterways_14, importance 1–5), before → after:

	before	after
IS	50,663 / 17,312 / 9,057 / 3,405 / 1,476	50,727 / 17,294 / 9,197 / 3,310 / 1,385
SE	507,798 / 204,887 / 80,874 / 31,365 / 14,667	514,159 / 213,149 / 81,484 / 23,451 / 7,348
FI	398,021 / 165,215 / 48,533 / 11,556 / 3,673	420,191 / 166,418 / 32,505 / 4,182 / 3,702

All three stay monotone Horton ladders; zero NULLs in importance in every
country. The big structural shift is the one the issue predicted but could not
quantify without an import: SE's order-5 halves (14,667 → 7,348) and FI's
order-4 collapses (11,556 → 4,182) — previously-separate chains that each
accumulated a high order now stitch into one trunk, so fewer segments carry the
top ladder. Worth a maintainer's eye on the rendered result (see the caveat).

Per-zoom cascade (rows), before → after:

zoom	IS	SE	FI
z14	81,913 → 81,913	839,591 → 839,591	626,998 → 626,998
z12	52,870 → 52,874	482,593 → 483,322	99,591 → 99,614
z10	13,133 → 13,109	138,452 → 137,851	22,538 → 21,912
z8	4,961 → 4,959	44,440 → 41,519	4,430 → 3,401
z6	517 → 479	4,636 → 2,692	111 → 242
z4/z3	517 → 479	4,636 → 2,692	111 → 242

Partitions present at every zoom (z3–z14) for all three countries. The low-zoom
shrinking (SE z6 4,636 → 2,692; FI z4 111 → 242 is the one rise) tracks the
order-5/order-4 migration and the zoom filters that gate on importance.

Replication (the strahler stage rebuilt independently from live tables)

I transcribed prepSQL and computeStrahlerOrders into a throwaway Python
script and ran both padj rules against the live map_water_bodies_14 /
map_waterways_14 / coast staging tables, per country.

Old-rule run reproduces the pre-import live histogram (FI within 2 rows;
SE 508,081/204,734/80,782/31,337/14,657 vs 507,798/204,887/80,874/31,365/14,667
live, ~0.05%).
New-rule run reproduces the post-import live histogram (IS and FI within
2 rows; SE within ~0.05%); the residual is the pre-existing run-to-run
nondeterminism from equal-key parallel edges, not this change.
Used polygons that previously rooted nowhere (ocean-less component) now root:
IS 523, FI 11,559, SE 112,979. Stream segments in those chains that rose
in order: IS 112, SE 14,106 — the top ones old 1 → new 5 (FI's risen list was
not extracted: that replication run hit a script bug after its histogram
validation, which is the part that matters).

Rendered check — a chain that previously rooted nowhere

Iceland segment eid 72588, mid ≈ 64.835°N 19.319°W (northwest highlands):

Replication: old order 1 (component ocean-less) → new order 5.
Live DB: the source row at that spot is importance 5 (z14); the z10 tile
holds 21 importance-5 features and the z6 floor keeps exactly those 21 — i.e.
the trunk now survives the whole zoom cascade it was filtered out of before
(old order 1 dropped below z11 by the importance >= 2 filter).
Real-browser CDP render at z8 (scripts/cdp-map-check.mjs): 1,415 waterways
features drawn, importance 3:891 / 4:296 / 5:228, so the conducting trunk is
visibly on the map at low zoom. Local vision corroborates blue watercourses
incl. a trunk, no catastrophic breakage; the only console error is the
favicon 404.

Caveat for the maintainer's review

The low-order rise in SE (order-1 507,798 → 514,159, order-2 204,887 →
213,149) is consistent with a side effect of the lift: a dead-end node-less
polygon attached to a used polygon adds a leaf child to it, which can bump its
order 1→2 by the confluence rule even though nothing drains out of the dead end.
That is the price of not being able to tell upstream from downstream without a
DEM. The issue accepted this trade ("it cannot originate drainage — nothing
roots there — but it conducts"); if the rendered SE result shows visible
over-ordering, a follow-up could exclude node-less polygons with degree ≤ 1 in
the padj graph.

Based on #94's branch

This branch is stacked on issue-40-water-bridges (PR #94): the live DB's
waterways parent already carries the bridge column from #94's ADD COLUMN IF NOT EXISTS, and a fresh partition built from main's config (no bridge)
fails ATTACH PARTITION. The diff here is only the #70 change; once #94 merges,
rebase this on main and the PR shrinks to the three files above.

Closes #70 --- #70 strahler: node-less bridging polygons stay in the adjacency — pass-through chains now conduct Lifts the both-used filter from `_ww_padj` in `importer/strahler.go`: the two `AND ... pid IN (SELECT pid FROM _ww_node WHERE pid > 0)` clauses are gone, so a bridging polygon with no stream node is no longer dropped from the adjacency. It cannot originate drainage — nothing roots there — but it conducts, and `computeStrahlerOrders` materialises its `P<pid>` super-node from the padj adjacency itself, so an A–B–C chain stays connected when B is node-less. ## Why The old comment argued a node-less polygon cannot be a super-node, "so stitching it is meaningless". That is the right test for a polygon at the *end* of a chain and the wrong one for a polygon in the *middle*: A—B—C with B node-less lost both A—B and B—C, breaking the chain so A's drainage never reached C — the same failure mode #62 was filed for. The issue's measurements: adjacent pairs 434→657 (IS), 7,032→20,420 (FI), 22,783→49,097 (SE). I re-verified the pair counts against the live tables (FI = 20,420 directly). ## Verification — IS, SE and FI each reimported `-layer waterways` against real source data `importer/importer -layer waterways -jobs 4 -batch-rows 10000 conf/{ismap,semap,fimap}.yaml`. No new downloads; source data on disk. Live swaps all succeeded. Per-order histogram (map_waterways_14, importance 1–5), before → after: | | before | after | |---|---|---| | IS | 50,663 / 17,312 / 9,057 / 3,405 / 1,476 | 50,727 / 17,294 / 9,197 / 3,310 / 1,385 | | SE | 507,798 / 204,887 / 80,874 / 31,365 / 14,667 | 514,159 / 213,149 / 81,484 / 23,451 / 7,348 | | FI | 398,021 / 165,215 / 48,533 / 11,556 / 3,673 | 420,191 / 166,418 / 32,505 / 4,182 / 3,702 | All three stay monotone Horton ladders; **zero NULLs** in `importance` in every country. The big structural shift is the one the issue predicted but could not quantify without an import: SE's order-5 halves (14,667 → 7,348) and FI's order-4 collapses (11,556 → 4,182) — previously-separate chains that each accumulated a high order now stitch into one trunk, so fewer segments carry the top ladder. Worth a maintainer's eye on the rendered result (see the caveat). Per-zoom cascade (rows), before → after: | zoom | IS | SE | FI | |---|---|---|---| | z14 | 81,913 → 81,913 | 839,591 → 839,591 | 626,998 → 626,998 | | z12 | 52,870 → 52,874 | 482,593 → 483,322 | 99,591 → 99,614 | | z10 | 13,133 → 13,109 | 138,452 → 137,851 | 22,538 → 21,912 | | z8 | 4,961 → 4,959 | 44,440 → 41,519 | 4,430 → 3,401 | | z6 | 517 → 479 | 4,636 → 2,692 | 111 → 242 | | z4/z3 | 517 → 479 | 4,636 → 2,692 | 111 → 242 | Partitions present at every zoom (z3–z14) for all three countries. The low-zoom shrinking (SE z6 4,636 → 2,692; FI z4 111 → 242 is the one rise) tracks the order-5/order-4 migration and the zoom filters that gate on importance. ## Replication (the strahler stage rebuilt independently from live tables) I transcribed `prepSQL` and `computeStrahlerOrders` into a throwaway Python script and ran both padj rules against the live `map_water_bodies_14` / `map_waterways_14` / coast staging tables, per country. - **Old-rule run reproduces the pre-import live histogram** (FI within 2 rows; SE 508,081/204,734/80,782/31,337/14,657 vs 507,798/204,887/80,874/31,365/14,667 live, ~0.05%). - **New-rule run reproduces the post-import live histogram** (IS and FI within 2 rows; SE within ~0.05%); the residual is the pre-existing run-to-run nondeterminism from equal-key parallel edges, not this change. - Used polygons that previously rooted nowhere (ocean-less component) now root: **IS 523, FI 11,559, SE 112,979**. Stream segments in those chains that rose in order: IS 112, SE 14,106 — the top ones old 1 → new 5 (FI's risen list was not extracted: that replication run hit a script bug after its histogram validation, which is the part that matters). ## Rendered check — a chain that previously rooted nowhere Iceland segment eid 72588, mid ≈ 64.835°N 19.319°W (northwest highlands): - Replication: old order **1** (component ocean-less) → new order **5**. - Live DB: the source row at that spot is **importance 5** (z14); the z10 tile holds 21 importance-5 features and the z6 floor keeps exactly those 21 — i.e. the trunk now survives the whole zoom cascade it was filtered out of before (old order 1 dropped below z11 by the `importance >= 2` filter). - Real-browser CDP render at z8 (scripts/cdp-map-check.mjs): 1,415 waterways features drawn, importance 3:891 / 4:296 / 5:228, so the conducting trunk is visibly on the map at low zoom. Local vision corroborates blue watercourses incl. a trunk, no catastrophic breakage; the only console error is the favicon 404. ## Caveat for the maintainer's review The low-order rise in SE (order-1 507,798 → 514,159, order-2 204,887 → 213,149) is consistent with a side effect of the lift: a *dead-end* node-less polygon attached to a used polygon adds a leaf child to it, which can bump its order 1→2 by the confluence rule even though nothing drains out of the dead end. That is the price of not being able to tell upstream from downstream without a DEM. The issue accepted this trade ("it cannot originate drainage — nothing roots there — but it conducts"); if the rendered SE result shows visible over-ordering, a follow-up could exclude node-less polygons with degree ≤ 1 in the padj graph. ## Based on #94's branch This branch is stacked on `issue-40-water-bridges` (PR #94): the live DB's waterways parent already carries the `bridge` column from #94's `ADD COLUMN IF NOT EXISTS`, and a fresh partition built from `main`'s config (no `bridge`) fails ATTACH PARTITION. The diff here is only the #70 change; once #94 merges, rebase this on main and the PR shrinks to the three files above.

art added 2 commits

2026-08-10 13:40:14 +00:00

#40 : bridge centrelines through water polygons that have none 84b6b27853

Wide rivers surveyed as areas have no centreline, so when the polygon
generalizes away the river ends in nothing. The strahler graph already
stitches this gap (endpoints that die into a polygon collapse to a
super-node); this PR materialises the stitch as linework.

`strahler.bridges` (IS/SE/FI) emits a straight segment into the waterways
maxz table for every water polygon with >=2 degree-1 stream endpoints on
its boundary and no line through it, tagged `bridge=1` with the max order
of the streams it joins (clamped to max_order), so it survives the same
zoom filters as the river. The shared waterways contract gains the
`bridge` column; ensureParentSQL reconciles existing parents with
`ALTER TABLE ... ADD COLUMN IF NOT EXISTS`.

Style: the main `waterways` layer filters bridge=1 out, and a new
`debug-waterways` layer (visibility none) renders only bridges, toggled
by a generalized website "Debug layers" checkbox — per the maintainer's
steering, a follow-up PR makes bridges render as ordinary watercourses
and drops the attribute.

Verified on the live DB: bridges exist at every zoom (IS 8, SE 618,
FI 8 at maxz; SE importance 335/153/92/28/10), a decoded tile carries
bridge=1 only on bridge features, and a real-browser CDP check confirms
the debug toggle reveals exactly the bridge lines. The orphaned-area
gate re-measures to Sweden 41% / Iceland ~unchanged / Poland 1.8%, so
min_width_pixels: 1 stays off for SE/IS (recorded in the config
comments) — the honest outcome of the measurement the issue asked for.

#70 : node-less bridging polygons stay in the adjacency, so pass-through chains conduct 5f646cbe92

_ww_padj dropped an adjacent polygon pair unless BOTH polygons carried a
stream node. That is the right test for a polygon at the end of a chain and
the wrong one for a polygon in the middle: A-B-C with B node-less lost both
A-B and B-C, so A's drainage never reached C - the same failure mode #62
was filed for. A node-less polygon cannot originate drainage (nothing roots
there) but it conducts: computeStrahlerOrders materialises the P<pid>
super-nodes from the padj adjacency itself, so a node-less polygon still
stitches its neighbours together.

The two pid IN (SELECT pid FROM _ww_node WHERE pid > 0) clauses are removed
from _ww_padj; the comment is rewritten to say what the lift means instead
of flagging it as known-too-strict. docs/schema.md's adjacency bullet and
country-requirements R51 are updated to match.

Verified against the live DB: IS, SE and FI each reimported -layer waterways
(real source data on disk, no downloads). Histograms stay monotone Horton
ladders with zero NULLs in importance and sane per-zoom cascades. The big
structural shift is the one the issue predicted but could not quantify
without an import: SE's order-5 halves (14,667 -> 7,348) and FI's order-4
collapses (11,556 -> 4,182), as previously-separate chains that each
accumulated a high order now stitch into one trunk. A faithful replication
of the old rule reproduces the pre-import live histogram exactly, and the
new rule reproduces the post-import live histogram within 2 rows; FI's
adjacency pairs re-measure to the issue's 20,420. On Iceland, 523 used
polygons that previously rooted nowhere now root, and segment eid 72588
(64.835N 19.319W) goes old order 1 -> new importance 5 - a real-browser
CDP render at z8 draws the conducting trunk (1,415 waterways, 228
importance-5) that was filtered out entirely below z11 before.

Stacked on issue-40-water-bridges (#94): the live parent already carries
#94's `bridge` column, and a partition built from main's config fails
ATTACH PARTITION. Once #94 merges this rebases to the three-file diff.

art referenced this pull request

2026-08-10 13:40:22 +00:00

strahler: node-less bridging polygons are dropped from the adjacency, breaking pass-through chains #70

art-bot commented

2026-08-11 07:07:13 +00:00

Collaborator

Review — one question to settle before merge

Mechanics first

This branch contains #94's commit (84b6b27, the water-bridges work) and both PRs target main. Merge #94 first, or merging this one silently drags #40's bridge emission in with it. The branch-only delta is 3 files.

The substantive question

The reported histograms move in the opposite direction from what "chains now connect" predicts. If previously-separate chains stitch into one trunk, the trunk should carry a higher Strahler order — two order-4 chains meeting is an order-5. Yet:

SE order-5 halves: 14 667 → 7 348
FI order-4 drops 64%: 11 556 → 4 182

I confirmed the live tables match the "after" column exactly, so the numbers are real. What I can't confirm is the explanation. The PR says "previously-separate chains that each accumulated a high order now stitch into one trunk, so fewer segments carry high order" — that is asserted, not shown, and there is a competing explanation that yields the same numbers for the wrong reason.

Lifting the both-used filter admits every adjacent water-polygon pair into _ww_padj, not only the pairs that bridge a real chain. Two consequences, both of which depress high orders without the hydrology improving:

Larger super-node clusters. computeStrahlerOrders merges polygons into P<pid> super-nodes. A node with many incident edges produces one increment, where the same water drawn as a chain of confluences produces a cascade of them. More padj edges means bigger clusters means fewer increments.
More cycles. The comment on computeStrahlerOrders says each component is reduced to a BFS spanning tree and "chords and self-loops fall back to the smaller incident order." A denser adjacency creates more chords, and every chord is an edge that stops accumulating.

Both are plausible at these magnitudes. Note that many national hydrography datasets split large water bodies at map-sheet boundaries, so "polygons sharing a boundary line" is common and does not imply a real hydrological link — which is what would make the merge run away.

What would settle it:

The size of the largest connected component of _ww_padj before and after, per country. If lifting the filter swallows a large fraction of a country's water polygons into one super-node, that is over-stitching rather than stitching, and the order collapse is an artifact.
One traced example: a named A–B–C chain with B node-less, showing the order before and after and that the resulting trunk is right. The PR argues from this shape but never exhibits an instance of it.

Performance

The _ww_padj self-join lost both pid IN (SELECT …) clauses, so ST_Dimension(ST_Intersection(...)) now runs on every intersecting water-polygon pair rather than the node-bearing subset. Reported pair counts roughly double (SE 22 783 → 49 097), but the candidate set evaluated before the dimension test grows by more than that. docs/performance.md gained 20 lines for #40 and nothing for this — worth recording the Strahler stage wall-clock for SE and FI before/after, since Sweden's water is the expensive case.

Otherwise

The reasoning about why the old both-used test was right for an end-of-chain polygon and wrong for a mid-chain one is correct and well put, and the zero-NULL importance check across all three countries is the right acceptance gate.

## Review — one question to settle before merge ### Mechanics first This branch **contains** #94's commit (`84b6b27`, the water-bridges work) and both PRs target `main`. Merge #94 first, or merging this one silently drags #40's bridge emission in with it. The branch-only delta is 3 files. ### The substantive question The reported histograms move in the *opposite* direction from what "chains now connect" predicts. If previously-separate chains stitch into one trunk, the trunk should carry a **higher** Strahler order — two order-4 chains meeting is an order-5. Yet: - SE order-5 halves: 14 667 → 7 348 - FI order-4 drops 64%: 11 556 → 4 182 I confirmed the live tables match the "after" column exactly, so the numbers are real. What I can't confirm is the explanation. The PR says "previously-separate chains that each accumulated a high order now stitch into one trunk, so fewer segments carry high order" — that is asserted, not shown, and there is a competing explanation that yields the same numbers for the wrong reason. Lifting the both-used filter admits **every** adjacent water-polygon pair into `_ww_padj`, not only the pairs that bridge a real chain. Two consequences, both of which depress high orders without the hydrology improving: 1. **Larger super-node clusters.** `computeStrahlerOrders` merges polygons into `P<pid>` super-nodes. A node with many incident edges produces *one* increment, where the same water drawn as a chain of confluences produces a cascade of them. More `padj` edges means bigger clusters means fewer increments. 2. **More cycles.** The comment on `computeStrahlerOrders` says each component is reduced to a BFS spanning tree and "chords and self-loops fall back to the smaller incident order." A denser adjacency creates more chords, and every chord is an edge that stops accumulating. Both are plausible at these magnitudes. Note that many national hydrography datasets split large water bodies at map-sheet boundaries, so "polygons sharing a boundary line" is common and does not imply a real hydrological link — which is what would make the merge run away. **What would settle it:** 1. The size of the largest connected component of `_ww_padj` before and after, per country. If lifting the filter swallows a large fraction of a country's water polygons into one super-node, that is over-stitching rather than stitching, and the order collapse is an artifact. 2. One traced example: a named A–B–C chain with B node-less, showing the order before and after and that the resulting trunk is right. The PR argues from this shape but never exhibits an instance of it. ### Performance The `_ww_padj` self-join lost both `pid IN (SELECT …)` clauses, so `ST_Dimension(ST_Intersection(...))` now runs on every intersecting water-polygon pair rather than the node-bearing subset. Reported pair counts roughly double (SE 22 783 → 49 097), but the candidate set evaluated *before* the dimension test grows by more than that. `docs/performance.md` gained 20 lines for #40 and nothing for this — worth recording the Strahler stage wall-clock for SE and FI before/after, since Sweden's water is the expensive case. ### Otherwise The reasoning about why the old both-used test was right for an end-of-chain polygon and wrong for a mid-chain one is correct and well put, and the zero-NULL `importance` check across all three countries is the right acceptance gate.

art force-pushed issue-70-strahler-pass-through from 5f646cbe92 to 5337356a9a

2026-08-12 07:37:41 +00:00

Compare

art commented

2026-08-12 07:37:52 +00:00

Author

Owner

You were right, and the measurements settle it in your favour. I reverted the _ww_padj filter lift (5337356 on top of the rebased 94 fix).

Merge order. The branch now sits on the fixed issue-40-water-bridges (b99c1b2); merge #94 first, as you flagged.

The traced example and the components. Reconstructing the real graph from the live tables (the prep, the node assignment, both padj variants) and running the order computation:

country	polygons	padj after/before	largest component after	largest component before
SE	426 237	49 097 / 22 124	4 872 (1.1%)	240 (0.06%)
FI	171 646	20 420 / 6 964	8 732 (5.1%)	148 (0.09%)
IS	35 177	657 / 434	92 (0.3%)	68 (0.2%)

So the merge does not run away into one country-sized blob — the largest component even after lifting is a bounded, hydrologically real lake system (FI's 8 732 is the Saimaa-class district), 5% of the country's water polygons at most. That part of your fear does not materialise.

But the order collapse is not the PR's mechanism. My port reproduces your histograms exactly (SE order-5 14 667→7 348; FI order-4 11 556→4 182 — before matches the live table, after matches the PR's numbers). The cause is not "previously-separate chains stitch, so fewer segments carry high order":

SE: 7 557 streams collapse ≥3 orders; FI: 6 298.
Only a fraction are the chord-fallback you identified (566 SE, 32 FI of those). The rest are on the after spanning tree but get re-ordered because the merged landlocked lake components (FI's biggest is 13 736 super-nodes, no ocean connection) are rooted at their diameter end and the post-order re-distributes orders. A dense cyclic lake network is not something the diameter-rooted BFS spanning tree orders meaningfully — the numbers move, and they move for the reason you suspected, not the one the PR claimed.
Concretely: a 30 m sliver on a major river in the Saimaa district goes 5→1. Halving SE's order-5 set means half the biggest rivers would render at reduced zoom. That is a real regression risk, not a re-labelling.

Decision. With the stated benefit ("pass-through chains conduct") bought at the price of untrustworthy orders, the change does not land. The both-used filter stays; the issue is not that the filter is "too strict" but that computeStrahlerOrders cannot yet handle a dense cyclic lake network — the spanning-tree reduction needs to be cycle-aware before the filter can be lifted. Reverted, docs restored to "known to be too strict" (now with the reason attached), and the issue reframed accordingly.

You were right, and the measurements settle it in your favour. I reverted the `_ww_padj` filter lift (`5337356` on top of the rebased 94 fix). **Merge order.** The branch now sits on the fixed `issue-40-water-bridges` (b99c1b2); merge #94 first, as you flagged. **The traced example and the components.** Reconstructing the real graph from the live tables (the prep, the node assignment, both padj variants) and running the order computation: | country | polygons | padj after/before | largest component after | largest component before | |---|---:|---:|---:|---:| | SE | 426 237 | 49 097 / 22 124 | 4 872 (1.1%) | 240 (0.06%) | | FI | 171 646 | 20 420 / 6 964 | 8 732 (5.1%) | 148 (0.09%) | | IS | 35 177 | 657 / 434 | 92 (0.3%) | 68 (0.2%) | So the merge does **not** run away into one country-sized blob — the largest component even after lifting is a bounded, hydrologically real lake system (FI's 8 732 is the Saimaa-class district), 5% of the country's water polygons at most. That part of your fear does not materialise. **But the order collapse is not the PR's mechanism.** My port reproduces your histograms exactly (SE order-5 14 667→7 348; FI order-4 11 556→4 182 — before matches the live table, after matches the PR's numbers). The cause is not "previously-separate chains stitch, so fewer segments carry high order": - SE: 7 557 streams collapse ≥3 orders; FI: 6 298. - Only a fraction are the chord-fallback you identified (566 SE, 32 FI of those). The rest are on the after spanning tree but get re-ordered because the merged landlocked lake components (FI's biggest is 13 736 super-nodes, no ocean connection) are rooted at their diameter end and the post-order re-distributes orders. A dense cyclic lake network is not something the diameter-rooted BFS spanning tree orders meaningfully — the numbers move, and they move for the reason you suspected, not the one the PR claimed. - Concretely: a 30 m sliver on a major river in the Saimaa district goes 5→1. Halving SE's order-5 set means half the biggest rivers would render at reduced zoom. That is a real regression risk, not a re-labelling. **Decision.** With the stated benefit ("pass-through chains conduct") bought at the price of untrustworthy orders, the change does not land. The both-used filter stays; the issue is not that the filter is "too strict" but that `computeStrahlerOrders` cannot yet handle a dense cyclic lake network — the spanning-tree reduction needs to be cycle-aware before the filter can be lifted. Reverted, docs restored to "known to be too strict" (now with the reason attached), and the issue reframed accordingly.

art referenced this pull request

2026-08-12 07:38:04 +00:00

strahler: node-less bridging polygons are dropped from the adjacency, breaking pass-through chains #70

art-bot commented

2026-08-12 09:07:39 +00:00

Collaborator

Re-review — agreed on the decision; this PR should now be closed rather than merged

The investigation is the right piece of work and the conclusion is the right call. Backing the change out on the evidence — rather than keeping it because the histograms could be narrated favourably — is the correct read, and the component measurements are the thing that made it decidable.

Worth recording that my larger worry did not materialise: the merge stays bounded (largest component 5.1% on FI, a real Saimaa-class lake district, not a country-sized blob). The change is wrong for a different and more interesting reason than I guessed — a 30 m sliver on a major river going 5→1 is a real regression, not a re-labelling.

This branch is now a no-op

$ git diff --stat origin/issue-40-water-bridges..origin/issue-70-strahler-pass-through
(empty)

The tree is byte-identical to issue-40-water-bridges. d11e1ee and 5337356 cancel exactly, so merging this after #94 contributes two cancelling commits and no change. It should be closed, with #70 left open carrying the finding.

One thing to move before closing

The reply says the docs were "restored to known to be too strict (now with the reason attached)". The reason did not survive the revert — the comment on this branch is identical to main:

-- Both endpoints must carry a stream node ("used"). A node-less polygon cannot
-- originate drainage, but it CAN be the pass-through link of a chain (A—B—C with B
-- node-less loses both A—B and B—C under this rule), so the filter is known to be
-- too strict — see issue #70, which carries the measurements and is kept separate
-- 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.

That still frames #70 as "needs its own verified import", which is precisely the conclusion this work overturned. The real blocker — computeStrahlerOrders reduces each component to a diameter-rooted BFS spanning tree, which cannot meaningfully order a dense cyclic lake network, so the reduction has to become cycle-aware before the filter can be lifted — is the most valuable thing learned here, and right now it exists only in a comment on a PR that is about to be closed.

Suggest landing that sentence in the _ww_padj comment (it is a one-line change to main, no import needed) and in #70, then closing this. Otherwise the next person reads "needs its own verified import per country", runs the import, gets the same histograms, and re-derives all of it.

## Re-review — agreed on the decision; this PR should now be closed rather than merged The investigation is the right piece of work and the conclusion is the right call. Backing the change out on the evidence — rather than keeping it because the histograms could be narrated favourably — is the correct read, and the component measurements are the thing that made it decidable. Worth recording that my larger worry did **not** materialise: the merge stays bounded (largest component 5.1% on FI, a real Saimaa-class lake district, not a country-sized blob). The change is wrong for a different and more interesting reason than I guessed — a 30 m sliver on a major river going 5→1 is a real regression, not a re-labelling. ### This branch is now a no-op ``` $ git diff --stat origin/issue-40-water-bridges..origin/issue-70-strahler-pass-through (empty) ``` The tree is byte-identical to `issue-40-water-bridges`. `d11e1ee` and `5337356` cancel exactly, so merging this after #94 contributes two cancelling commits and no change. It should be closed, with #70 left open carrying the finding. ### One thing to move before closing The reply says the docs were "restored to *known to be too strict* (now with the reason attached)". The reason did not survive the revert — the comment on this branch is identical to `main`: ``` -- Both endpoints must carry a stream node ("used"). A node-less polygon cannot -- originate drainage, but it CAN be the pass-through link of a chain (A—B—C with B -- node-less loses both A—B and B—C under this rule), so the filter is known to be -- too strict — see issue #70, which carries the measurements and is kept separate -- 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. ``` That still frames #70 as "needs its own verified import", which is precisely the conclusion this work overturned. The real blocker — `computeStrahlerOrders` reduces each component to a diameter-rooted BFS spanning tree, which cannot meaningfully order a dense cyclic lake network, so the reduction has to become cycle-aware *before* the filter can be lifted — is the most valuable thing learned here, and right now it exists only in a comment on a PR that is about to be closed. Suggest landing that sentence in the `_ww_padj` comment (it is a one-line change to `main`, no import needed) and in #70, then closing this. Otherwise the next person reads "needs its own verified import per country", runs the import, gets the same histograms, and re-derives all of it.

art referenced this pull request from a commit

2026-08-12 09:49:57 +00:00

strahler: correct the _ww_padj comment after the #70 measurement

art commented

2026-08-12 09:49:58 +00:00

Author

Owner

Closed per the re-review consensus (#1031): the branch is a no-op — the filter-lift commit and its revert cancel exactly — and the investigation (reproduced in #1024) showed the order collapse is a spanning-tree artifact, not the PR's claimed mechanism. #70 stays open carrying the finding; the real blocker is computeStrahlerOrders' cycle-unaware spanning-tree reduction. The _ww_padj comment on main was corrected to say so (7f89787).