[sumitchintanwar]

Overview
This PR introduces a "reuse" feature to r.watershed, allowing users to skip the computationally expensive flow accumulation and drainage direction steps if these maps already exist. This is particularly useful for:

• Iterative testing of different basin threshold values.
• Batch processing where the underlying elevation/flow model remains constant.

Key Changes

• Core Logic: Modified main.c and RAM/SEG modules to accept existing accumulation and drainage maps as inputs.
• Documentation: Added raster/r.watershed/REUSE_FEATURE.md covering workflows, limitations, and "Do's and Don'ts".
• Benchmarks: Added raster/r.watershed/testsuite/BENCHMARKS.md using hyperfine.
• Testing: Added new tests in testsuite/ to verify correctness.

Performance
Benchmarks performed using hyperfine on the NC SPM dataset show significant speedups when iterating on thresholds.

Multiple Benchmark tests using hyperfine show similar significant improvement like above.

Limitations

• Input Compatibility: As noted in the documentation, input maps must be generated by r.watershed. Maps from r.stream.* or other tools may have different drainage conventions and are not supported (explicitly warned in docs).
• Basin Delineation Only: The reuse mode focuses on recalculating basins/streams based on thresholds; it does not re-compute flow physics.

addressed Feedback

• Replaced time with hyperfine for statistically significant benchmarking.
• Added safety checks for flags (G_option_collective).
• Added comprehensive documentation and tests.

Closes #6720

[echoix]

Did you ever try using hyperfine for actually making the stats, doing enough runs for it to be statistically significative, handling outliers (often caused by other programs running), and removing the shell startup time.
With the timings you shown, using time maybe has limits since it wasn't really long. The "~4 times" is on user-mode CPU time only, but I'm a little sceptical that it isn't just variations between runs.

But any improvement is an improvement!

[petrasovaa]

Thanks! Please check our contributing guidelines for code style (use pre-commit).

Please add a test. Include a quick test (on smaller part of "elevation" map and maybe on artificial surface) and perhaps one that runs longer with larger area and deactivate it to not slow down the CI, but I can then at least run it locally.

I am not an expert on this tool, but one of my concerns with this is that if user tries to pass a flow accumulation/drainage raster computed with different tool (e.g. r.stream.* tools can handle that), the algorithm may potentially fail (incorrect results, segfaults). This can be of course discouraged in the documentation, but maybe there are other ways, e.g. detect the drainage conventions and check they match.

[sumitchintanwar]

Did you ever try using hyperfine for actually making the stats, doing enough runs for it to be statistically significative, handling outliers (often caused by other programs running), and removing the shell startup time. With the timings you shown, using time maybe has limits since it wasn't really long. The "~4 times" is on user-mode CPU time only, but I'm a little sceptical that it isn't just variations between runs.

But any improvement is an improvement!

I haven't tried hyperfine. Thank you for the suggestion. I will try that for the tests. i think the actual speedup would be more modest once measured correctly with hyperfine. But hey, It's still a worthwhile improvement.

[sumitchintanwar]

Thanks! Please check our contributing guidelines for code style (use pre-commit).

Please add a test. Include a quick test (on smaller part of "elevation" map and maybe on artificial surface) and perhaps one that runs longer with larger area and deactivate it to not slow down the CI, but I can then at least run it locally.

I am not an expert on this tool, but one of my concerns with this is that if user tries to pass a flow accumulation/drainage raster computed with different tool (e.g. r.stream.* tools can handle that), the algorithm may potentially fail (incorrect results, segfaults). This can be of course discouraged in the documentation, but maybe there are other ways, e.g. detect the drainage conventions and check they match.

@petrasovaa, Thanks for the feedback and the code review. I'll check the guidelines and get back to you with proper tests.

[sumitchintanwar]

Hey @petrasovaa, @echoix, I have made the requested changes and also added benchmarks using hyperfine along with proper documentation, tests and constraints of this feature. Ready for review when you have a chance!

[sumitchintanwar]

On a side note, I have to admit, I'm still climbing the GRASS learning curve a bit, but I'm really enjoying the challenge! It's super interesting digging into how this all works. Let me know what you think of the updates!"

[sumitchintanwar]

Some of the checks are failing because of hyperfine not found. Is there anything I should do to resolve this specifically?

"raster/r.watershed/testsuite/benchmark_reuse.sh: line 15: hyperfine: command not found"

[petrasovaa]

Given the significant changes in the code, I think we need to step back and write proper tests of the current code in a separate PR. I don't think the existing tests are comprehensive enough. Once those tests are merged, we can verify this PR is not breaking anything. Limit the region so that the test run fast, but include a test with larger area that we can verify locally but skip it in the CI.

[sumitchintanwar]

Given the significant changes in the code, I think we need to step back and write proper tests of the current code in a separate PR. I don't think the existing tests are comprehensive enough. Once those tests are merged, we can verify this PR is not breaking anything. Limit the region so that the test run fast, but include a test with larger area that we can verify locally but skip it in the CI.

Okay. I understand @petrasovaa . I will make a separate PR with more comprehensive tests. I would like some clarity on what more tests should I add.

I have skipped Basin delineation, TCI Sp calculation as for those significant algorithm changes will have to be added.
I'm thinking we'll do after the current feature is approved.

[sumitchintanwar]

The Tests are for reuse functionality are added in PR #6992. I'd appreciate the review.

[petrasovaa]

Given the significant changes in the code, I think we need to step back and write proper tests of the current code in a separate PR. I don't think the existing tests are comprehensive enough. Once those tests are merged, we can verify this PR is not breaking anything. Limit the region so that the test run fast, but include a test with larger area that we can verify locally but skip it in the CI.

Okay. I understand @petrasovaa . I will make a separate PR with more comprehensive tests. I would like some clarity on what more tests should I add.

That's a misunderstanding... Please reread my comment again. To move on with this PR, we need to have tests in place that would catch any regressions of the existing functionality caused by this PR. This PR may potentially break existing functionality and need to be able to catch that. Existing tests are not comprehensive enough.

I have skipped Basin delineation, TCI Sp calculation as for those significant algorithm changes will have to be added. I'm thinking we'll do after the current feature is approved.

Could explain this a little bit more?

[sumitchintanwar]

That's a misunderstanding... Please reread my comment again. To move on with this PR, we need to have tests in place that would catch any regressions of the existing functionality caused by this PR. This PR may potentially break existing functionality and need to be able to catch that. Existing tests are not comprehensive enough.

I am sorry for the confusion. I misunderstood. I'll add tests for the current code in a different pr, so it is ensured that the existing functionality isn't broken.

[sumitchintanwar]

I have skipped Basin delineation, TCI Spi calculation as for those significant algorithm changes will have to be added. I'm thinking we'll do after the current feature is approved.

Could explain this a little bit more?

@petrasovaa
Basically, Basin outputs are dependent on threshold, different thresholds create different basin boundaries from the same flow data. This threshold-based processing is not yet implemented in reuse mode and can be planned for later.

TCI/SPI require slope computation from the elevation model, which is skipped when reusing flow maps.

To keep this change focused, reuse mode is currently limited to outputs that can be derived directly from reused flow maps.

[sumitchintanwar]

@petrasovaa The regression tests are added in PR #7029. I’d really appreciate a quick review.

[metzm]

The changes look good to me. The idea of re-using flow accumulation and drainage directions for different threshold values is certainly appealing and obviously speeds up calculations.

I wonder if r.stream.extract would not be a better place to implement this. The reasons are that r.stream.extract can already re-use flow accumulation, adding pre-computed drainage direction should not be too difficult. The interface of r.watershed has already a lot of options and flags, this PR adds even more options. The downside would be that (half-)basin delineation would need to be added to r.stream.extract at the cost of code duplication, but then the code base is very similar between r.watershed and r.stream.extract and it should not be too difficult, with the notable difference that stream IDs are different from the IDs assigned by r.watershed.

[sumitchintanwar]

The changes look good to me. The idea of re-using flow accumulation and drainage directions for different threshold values is certainly appealing and obviously speeds up calculations.

I wonder if r.stream.extract would not be a better place to implement this. The reasons are that r.stream.extract can already re-use flow accumulation, adding pre-computed drainage direction should not be too difficult. The interface of r.watershed has already a lot of options and flags, this PR adds even more options. The downside would be that (half-)basin delineation would need to be added to r.stream.extract at the cost of code duplication, but then the code base is very similar between r.watershed and r.stream.extract and it should not be too difficult, with the notable difference that stream IDs are different from the IDs assigned by r.watershed.

@metzm
Thanks for the feedback! I would prefer keeping this in r.watershed because multi-threshold analysis needs basins, half-basins and RUSLE factors, not just streams.
Also, Moving to r.stream.extract means duplicating basin delineation code, bug fixes would need updating in two places.
Wouldn't the different ID schemes between r.watershed and r.stream.extract create confusion when comparing results across thresholds?

Happy to discuss if you feel strongly otherwise!

[metzm]

The main argument against moving this functionality to r.stream.extract is indeed code duplication and various changes needed to r.stream.extract. Comparing to this PR, this would require many more changes. I am happy with the current proposed changes to r.watershed, they add re-using previously computed results without modifying the code base too much.

[metzm]

Thanks for this contribution!

[petrasovaa]

Let's first merge the tests to avoid regressions we experienced in the past.

[sumitchintanwar]

@petrasovaa, I created a separate PR for the regression tests. The tests have been added in PR #7029, as you suggested earlier.

Given the significant changes in the code, I think we need to step back and write proper tests of the current code in a separate PR. I don't think the existing tests are comprehensive enough. Once those tests are merged, we can verify this PR is not breaking anything. Limit the region so that the test run fast, but include a test with larger area that we can verify locally but skip it in the CI.