fl_valley_attribute(): attribute floodplains per watercourse - #42
Merged
NewGraphEnvironment merged 6 commits intoAug 28, 2026
Merged
Conversation
Sync conventions from soul/conventions/ for LLM-assisted development. Adds cartography and pkgdown-publishing; repairs the corrupted soul conventions marker. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS
Relates to #40 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS
Failing tests that define per-group attribution: total coverage of valley cells, containment within the delineation, overlap preserved at confluences, the degenerate single-group case reproducing fl_valley_poly(), grouping invariance, and a no-crop oracle for corridor cropping. Encodes one thing the design has to handle: fl_valley_confine() adds cells after the mask intersection (cleanup, channel buffer, waterbodies — the last with no spatial filter), so a threshold test alone leaves orphan valley cells. Coverage needs a documented fallback, and complete = FALSE isolates the strict cover for testing. Relates to #40 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS
Attributes a finished delineation to the stream groups that produced it, without recomputing the delineation. A cell belongs to group g when it is a valley cell and satisfies, for g's streams alone, the two stream-dependent VCA criteria: within max_width/2 and cost-distance under cost_threshold. Rows overlap where ground is shared, which near a confluence it genuinely is. Per-group VCA runs were rejected on measurement, not taste: they disagree with the whole-network run in both directions (510 cells present in a group run and absent from the full run; 22 the reverse), so "the Morice floodplain" would depend on what else was in the run. Coverage is total by construction. fl_valley_confine() adds cells after intersecting its masks — cleanup, channel buffer, and waterbodies, the last with no spatial filter — so a threshold test alone orphans them (1,643 cells with the bundled waterbodies). Those are assigned to the nearest group, with the count reported and exposed as attr(x, "fl_fallback_cells"). Also fixes a latent bug in fl_valley_poly(): renaming an sf column by position detached the geometry column when as.polygons() returned no features, so fl_valley_poly() on a tile with no floodplain produced an sf that errored on every accessor. Same idiom that three review rounds kept finding; an internal assert on the return path now closes the family. Vignette gains a "Whose floodplain is it?" section. Relates to #40 Relates to NewGraphEnvironment/floodplains#40 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS
Outcome summary in the archive README: mechanism chosen on measurement, three code-check rounds plus a concurrent plan review, and the two follow-ups (#41, floodplains#40 driver half). Relates to #40 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS
New fl_valley_attribute(), plus the fl_valley_poly() empty-delineation fix. R CMD check: 0 errors, 0 warnings, 1 pre-existing NOTE (pkgdown/ at top level). 233 tests green. Relates to #40 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS
NewGraphEnvironment
deleted the
40-fl-valley-attribute-attribute-valley-cel
branch
August 28, 2026 02:07
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.
What
New
fl_valley_attribute(): takes a finishedfl_valley_confine()delineation and returns onesfrow per stream group, so a floodplain can be filtered and clicked per watercourse instead ofonly per network. Motivated by Morice fieldwork — standing on the ground there was no way to tell
whether a site was inside the Morice floodplain or upstream of it.
A cell belongs to group
gwhen it is a valley cell and satisfies, forg's streams alone, thetwo stream-dependent criteria the VCA already applies to the whole network — within
max_width / 2,and cost-distance under
cost_threshold. Rows overlap where ground is shared, which near aconfluence it genuinely is.
fl_valley_confine()is not modified.Package-side half of NewGraphEnvironment/floodplains#40, whose body is updated with the shipped API.
Why not run the VCA once per watercourse
Measured, not assumed. Only the slope mask is independent of which streams are supplied; the
distance mask, the cost distance, and the flood model (IDW over every seed cell) all move with the
stream set, and cleanup couples patches globally. On the bundled tile, by
gnis_name:Per-group runs are not a decomposition of the whole-network run, so "the Morice floodplain" would
depend on what else was in the run. That is a correctness argument, not a performance one.
Coverage is total by construction
fl_valley_confine()adds cells after intersecting its masks — cleanup, channel buffer, andwaterbodies, the last with no spatial filter at all — so a threshold test alone orphans them
(1,643 cells, 3.0%, with the bundled waterbodies). Those go to the nearest group, with the count
reported and exposed as
attr(x, "fl_fallback_cells").complete = FALSEexposes the strict cover.Review
A concurrent plan review plus three code-check rounds, all archived under
planning/archive/2026-08-issue-40-fl-valley-attribute/:were vacuous —
gnis_nameandblue_line_keyare a bijection on the test tile, so the"grouping invariance" test would have passed for any implementation. Replaced with a coarsening
test that is falsifiable.
sfin the empty-result branch that no test reached.fallback, so it named groups that were in the output with 39 ha of area.
sfcolumn by position — which turned upthe same latent bug in
fl_valley_poly()on an empty delineation. Fixed there too, with aninternal assert on the return path so the family cannot come back.
Also
fl_cost_distance()bug found during review: every zero-frictioncell is treated as a seed, not just stream cells. Deliberately not fixed here — it changes VCA
output on any DEM containing exact zeros and deserves its own diff.
Verification
R CMD check: 0 errors, 0 warnings, 1 pre-existing NOTE (pkgdown/at top level)lintr: no new lints in the touched filesFixes #40
🤖 Generated with Claude Code
https://claude.ai/code/session_01U9SAqmvFeENADk4rYcbtHS