fix(engine): bound the flowline layer at both ends - #56
Merged
prayaslashkari merged 1 commit intoSep 19, 2026
Merged
Conversation
|
🚅 Deployed to the explorer-app-pr-56 environment in sawgraph-explorer
1 service not affected by this PR
|
railway-app
Bot
temporarily deployed
to
sawgraph-explorer / explorer-app-pr-56
September 19, 2026 19:50
Destroyed
GET_FLOWLINE_GEOMETRIES traced a one-sided transitive closure: it seeded from the cells of every resolved anchor, widened each by sfTouches, and followed the network to its end. Nothing in the query referenced the targets, so an anchor sitting near a drainage divide pulled in the whole of the neighbouring basin. On "What facilities are upstream from PFOS samples in York County, ME" that drew 122 segments of the Merrimack and 21 of the Winnipesaukee, ~130km away in a basin no York sample drains from, plus the Presumpscot, the Suncook and the Powwow. Nothing looked broken. The map just had extra rivers on it, and they were real rivers. The closure is now intersected with the flowlines that reach the resolved targets. Measured live with the real answer sets (1,497 facilities, 200 sample points): 3,271 flowlines down to 2,516, in the same 7s. 755 removed, 0 added, so the fix can only take away flowlines that were never on a path from a facility to a sample. Of those, 684 are in a different basin and 71 lie below a sample, which is the one visible behaviour change: the drawn river now stops at the sample instead of running on to the sea. Written as an intersection of two DISTINCT closures. The direct membership test, `?flowline hyf:downstreamFlowPathTC? ?_flTarget`, leaves ?flowline unbound on the left and QLever times out joining on it (31s, "Join on ?flowline"). The reach is a bare TC rather than reflexive: `TC?` was measured and recovers 0 flowlines while costing 8s. The bounded branch gets the same intersection. At 25km it went 3,034 -> 2,516, matching the unbounded set, because connectivity dominates the distance cap at that range; at 5km the cap still bites (2,314), so the two constraints compose. The shipped bounded query still drew a Merrimack segment through its cap. scripts/check-flowline-scope.mts asserts both ends are bound for all 100 shapes that draw the layer, bounded and unbounded, and runs in CI.
prayaslashkari
force-pushed
the
fix-upstream-flowline
branch
from
September 19, 2026 19:55
af37612 to
b7bcc73
Compare
prayaslashkari
changed the base branch from
development
to
fix/fused-seed-order-and-obs-joins
September 19, 2026 19:55
railway-app
Bot
temporarily deployed
to
sawgraph-explorer / explorer-app-pr-56
September 19, 2026 19:55
Destroyed
prayaslashkari
merged commit Sep 19, 2026
1522403
into
fix/fused-seed-order-and-obs-joins
3 of 4 checks passed
This branch was successfully deployed
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
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.
Surfaced on "What facilities are upstream from PFOS samples?" scoped to York County, ME. The discovery steps find the right facilities; this layer drew the wrong rivers around them.
The bug
GET_FLOWLINE_GEOMETRIEStraced a one-sided transitive closure. It seeded from the cells of every resolved anchor, widened each bysfTouches, and followed the network to its end:{ SELECT DISTINCT ?s2cellus WHERE { VALUES ?facilityA { ... } ... } } ?upstream_flowline rdf:type hyf:HY_FlowPath ; spatial:connectedTo ?s2cellus ; hyf:downstreamFlowPathTC ?flowline .Nothing in it references the targets. An anchor sitting near a drainage divide pulls in the whole of the neighbouring basin.
Measured with the real answer sets (1,497 facilities, 200 sample points), the drawn layer contained:
No York County sample drains from any of them. Nothing looked broken; the map just had extra rivers on it, and they were real rivers.
The fix
Intersect the closure with the flowlines that actually reach the resolved targets.
Zero added means the fix can only take away flowlines that were never on a path from a facility to a sample. Merrimack, Winnipesaukee and Presumpscot all go to 0.
Of the 755 removed, 684 are in a different basin and 71 lie below a sample. That second group is the one visible behaviour change: the drawn river now stops at the sample instead of running on to the sea.
Why an intersection
The direct membership test is the obvious form and does not work:
?flowlineis unbound on the left, so QLever times out. TwoDISTINCTclosures joined on?flowlinerun in the same 7s the one-sided query took.The reach is a bare
TC, not reflexive.TC?was measured: it recovers 0 flowlines and costs 8s, because the segment a target sits on already arrives via another target upstream of it.Bounded traces
The same intersection is applied to the distance-capped branch. At 25km it went 3,034 to 2,516, matching the unbounded set, because connectivity dominates the cap at that range. At 5km the cap still bites (2,314), so the two constraints compose rather than one masking the other. Worth noting the shipped bounded query still drew a Merrimack segment straight through its cap.
Guardrail
scripts/check-flowline-scope.mtsasserts both ends are bound for all 100 shapes that draw the layer, bounded and unbounded, and runs in CI alongside the existing trace-direction check. Offline, runs in seconds.npm run check-trace-direction(2,068 checks) andnpm run check-step-labels(651 checks) still pass. The 15 lint errors on this branch are pre-existing in React components and identical before the change.Stacked on #55
Base is
fix/fused-seed-order-and-obs-joins, notdevelopment. Merge #55 first.The two fix different bugs and touch disjoint regions, but the dependency is real for verification: without #55 the discovery step OOMs at 26s on the York question, so there are no anchor IRIs to feed this layer and the change cannot be reproduced end to end. Stacking lets a reviewer check out this branch and run the whole question.
Rebasing onto #55 surfaced one interaction worth recording. #55 reworked
bindEntityInCellso thatsampleObservations: falseno longer drops every observation join; it now emits the lean per-filter ones. That fed straight into this PR's target-cell subselect, which re-derived the PFOS substance filter on top of an IRI list that was already the filtered answer set. Same 2,516 flowlines, 1s slower. Fixed by passing the bare block so only the cell-membership triple is emitted, which puts the query back to exactly the text measured above.All five self-checks pass on the combined branch: flowline scope (400), query joins (265, #55's own), trace direction (2,068), step labels (651), cache key (19).