Skip to content

Comments

r.stream.extract: fix stream delineation for binary flow accumulation#6543

Merged
petrasovaa merged 1 commit intoOSGeo:mainfrom
petrasovaa:r.stream.extract-fix
Oct 28, 2025
Merged

r.stream.extract: fix stream delineation for binary flow accumulation#6543
petrasovaa merged 1 commit intoOSGeo:mainfrom
petrasovaa:r.stream.extract-fix

Conversation

@petrasovaa
Copy link
Contributor

Fixes #6541.

This block got removed as part of compiler warnings cleanup #1248 with reasoning that part has been removed from r.watershed. But accumulation in r.stream.extract can be passed externally, so it makes sense the code is different. The actual warning was due to a misplaced parenthesis.

I tested it in the use case in #6541.

I also tested on NC elevation (there are no tests) and this changes the output vector slightly on several places:

image (black with this PR, yellow for current state)

r.watershed stream output (raster) matches the current state.

@metzm any idea?

@github-actions github-actions bot added raster Related to raster data processing C Related code is in C module labels Oct 24, 2025
@metzm
Copy link
Contributor

metzm commented Oct 26, 2025

This change would break the intention of r.stream.extract. The intention is to follow flow accumulation as much as possible to trace streams, e.g. use flow accumulation from r.terraflow or some external tool or some modified flow accumulation from r.watreshed (see example in the manual), but take advantage of the A* search of r.watershed which does not require DEM modifications to successfully extract stream networks.
A better solution would be to solve ties of equal flow accumulation by following elevation. I will propose a separate PR with this change.

@metzm
Copy link
Contributor

metzm commented Oct 26, 2025

Thinking about it, this change does not break the intention of r.stream.extract because it would still prefer to follow increasing flow accumulation. Therefore I suggest to apply both this PR and my PR #6554

@petrasovaa
Copy link
Contributor Author

Thinking about it, this change does not break the intention of r.stream.extract because it would still prefer to follow increasing flow accumulation. Therefore I suggest to apply both this PR and my PR #6554

I see your PR fixes the issue, so I wonder if this PR is needed and why was the code block there to start with?

@metzm
Copy link
Contributor

metzm commented Oct 27, 2025

I see your PR fixes the issue, so I wonder if this PR is needed and why was the code block there to start with?

As the code comment says

/* set main drainage direction to A* path if possible */

To elaborate, the intention of r.stream.extract is to use external flow accumulation for stream extraction, but if in doubt follow the A* search direction, i.e. if (fabs(wat_nbr[np_side]) >= max_acc).

Restoring this deleted code block with your PR in addition to my PR should make r.stream.extract robust for special cases as reported in #6541

@petrasovaa petrasovaa merged commit 0cb6d7f into OSGeo:main Oct 28, 2025
27 checks passed
@petrasovaa petrasovaa deleted the r.stream.extract-fix branch October 28, 2025 13:18
@github-actions github-actions bot added this to the 8.5.0 milestone Oct 28, 2025
@metzm
Copy link
Contributor

metzm commented Oct 28, 2025

Since this is a bugfix, backport to G84?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C Related code is in C module raster Related to raster data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Cross-flow issue from the r.stream.extract function

2 participants