Seed the cost surface only from stream cells (#41) - #45
Merged
NewGraphEnvironment merged 4 commits intoAug 28, 2026
Conversation
fl_cost_distance() encodes seeds by setting stream cells to 0 and calling terra::costDist(target = 0), which matches every zero cell -- so any cell whose friction was already exactly zero acted as a free cost source, contradicting the function's own documentation. Floor friction == 0 to 1e-6 before seeding, so zero means "stream cell" by construction. Flat ground stays cheap to cross (0.1 accumulated over a 100 km path at 10 m, against a default cost_threshold of 2500); it stops being a source. Deliberately == 0 rather than <= 0: costDist rejects a negative cost surface, and flooring negatives would have disabled that guard, turning meaningless input into plausible-looking output. Eight new tests, all synthetic -- the bundled tile has no exact-zero slope cells and cannot reach this failure mode, which is why the bug shipped. Three were confirmed red against the unfixed function first; the rest guard the opposite over-correction and terra's negative-friction check. No bundled or default-source result moves: MRDEM-30 over the same AOI also has zero exact-zero slope cells, and fl_valley_confine() returns the same 53,635 valley cells. Fixes #41 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS
Two real defects from concurrent review, one claim disproved by measurement. The over-correction guard was too weak to do its job. It asserted that crossing flat ground costs less than crossing sloped ground -- which any floor below the sloped friction satisfies, so a floor of 1 passed while costing 1e5 over a 100 km path, 40x a default cost_threshold. Replaced with a negligibility ratio: the flat traverse must come in under 3e-5 of the sloped equivalent. Verified by restoration -- floors of 1e-6 and 1e-4 pass, 1e-3 and 1 fail. NEWS and methodology.md claimed the package's default DEM source was unaffected, generalizing from a single small MRDEM-30 clip. The package's own pars_dem.tif contradicts it: 80 exact-zero slope cells in 10.7 M, moving the cost mask by 2,289 cells (214 ha, 0 added). Rewritten with both datasets and an explicit warning not to read either as a general guarantee. The review also reported the shipped pars_valleys.tif as stale. Measured against the output rather than the intermediate -- the shipped raster is the old code's own output, so it is its own oracle -- and it is bit-identical: 521,028 cells both ways, zero differing in either direction. The other three VCA criteria and morphological cleanup absorb all 2,289 cost-mask cells. No artifact needs regenerating. Also loosened the negative-friction test to match "negative" rather than terra's full message, which this package does not own. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS
NewGraphEnvironment
deleted the
41-fl-cost-distance-seeds-every-zero-friction-c
branch
August 28, 2026 06:50
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.
Fixes #41.
Summary
fl_cost_distance()encodes stream seeds by setting stream cells to0in the friction surface andcalling
terra::costDist(target = 0)— which seeds on every cell equal to zero. Any cell whosefriction was already exactly zero was therefore acting as a free cost source, contradicting the
function's own documentation.
Fixed by flooring
friction == 0to1e-6before seeding, so zero means "stream cell" byconstruction rather than by assumption.
Reproduction
Synthetic 50x50 grid at 10 m, friction 10% everywhere, one stream cell, a 6x6 exact-zero patch:
Two decisions worth flagging
== 0, not<= 0as the issue proposed. Probing foundterra::costDist()rejects a negativecost surface outright (
negative friction values not allowed). Flooring<= 0would have silentlydisabled that guard, converting meaningless input into plausible-looking output — the same failure
direction this fix exists to close.
== 0eliminates exactly the set that would be mistaken forseeds and nothing more. A test now asserts the guard survives.
No new argument.
1e-6stays an internal documented constant.fl_cost_distance()is exported,so a user in unusual friction units already has a one-line escape hatch — pre-floor their own raster.
That keeps this a pure bug fix: 0.4.0 → 0.4.1.
Blast radius — measured on both shipped DEMs
The fix can only raise a cost that was spuriously zero, so it strictly removes cells from the cost
mask and never adds. Whether that reaches the delineation depends on the dataset:
< 2500)dem.tif/slope.tif, 10 mpars_dem.tif(MRDEM-30, 30 m, 20.9 Mcell)So MRDEM-30 — the package default source — does produce exact zeros at watershed scale, and the
cost mask does move. The delineation does not, on either dataset:
fl_valley_confine()intersects cost with slope, distance and flood and then runs morphological cleanup, which absorbs
all 2,289 cells. Shipped vignette artifacts are still current — verified against
pars_valleys.tifdirectly, which is itself the old code's output and therefore its own oracle.That is a property of these two datasets, not a guarantee. Where cost is the binding criterion —
flatter terrain, a lax
slope_threshold, a largeflood_factor— the delineation will shrink.Also verified:
costDisttarget matching is exact equality (a 1e-14 cell reads 7.07e-14, not 0), so== 0is the right comparison; and on bundled data the zero set is now identical to the 1,607stream cells.
Tests
8 new tests, all synthetic — the bundled tile has no exact-zero slope cells and is structurally
incapable of reaching this failure mode, which is why the bug shipped. Three were confirmed red
against the unfixed function before the fix was written; the rest guard the opposite
over-correction and terra's negative-friction check, and pass in both states by design.
The over-correction guard needed a second pass. Its first form asserted that crossing flat ground
costs less than crossing sloped ground — which any floor below the sloped friction satisfies, so a
floor of 1 passed it, while costing 1e5 over a 100 km path (40× a default
cost_threshold).Replaced with a negligibility ratio, then verified by restoration: floors of 1e-6 and 1e-4 pass,
1e-3 and 1 fail.
The bundled-data test asserts its own premise (
sum(slope == 0) == 0) beside the behaviour, so afuture test-data swap fails on the line naming the real cause.
Review
One concurrent review round. Two real defects fixed on the branch (the weak guard above; NEWS and
methodology.mdoverclaiming that the default DEM source was unaffected, generalized from a singlesmall clip). One finding — that the shipped Parsnip raster was stale — was disproved by measuring
the output rather than the intermediate.
Checks
devtools::test()— 247 pass, 0 faildevtools::check()— 0 errors, 0 warnings, 1 pre-existing NOTE (pkgdown/)lintr::lint_package()— only two pre-existing lints, both outside this diff🤖 Generated with Claude Code
https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS