Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r.watershed: fix streams and basins #3140

Merged
merged 2 commits into from
Sep 11, 2023
Merged

Conversation

metzm
Copy link
Contributor

@metzm metzm commented Sep 6, 2023

The PR #2648 introduced a bug in r.watershed such that streams and basins are no longer correctly calculated. This PR fixes calculation of streams and basins in r.watershed.

@metzm metzm added bug Something isn't working raster Related to raster data processing C Related code is in C backport to 8.3 labels Sep 6, 2023
@metzm metzm added this to the 8.4.0 milestone Sep 6, 2023
@metzm metzm requested a review from nilason September 6, 2023 13:02
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Underlines the advantages with always-use-braces-policy!
(Could probably have been prevented by ClangFormat InsertBraces: true https://clang.llvm.org/docs/ClangFormatStyleOptions.html#insertbraces).

@metzm
Copy link
Contributor Author

metzm commented Sep 6, 2023

This could also have been prevented by stricter tests, adjusted in 0ec0004

@neteler
Copy link
Member

neteler commented Sep 6, 2023

This could also have been prevented by stricter tests, adjusted in 0ec0004

Thanks.

FYI: I tried the new test my unfixed copy and it fails as expected:

======================================================================
FAIL: test_thresholdsize (__main__.TestWatershed.test_thresholdsize)
Test the expected range of basin output values
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mneteler/software/grass_main/raster/r.watershed/testsuite/r_watershed_test.py", line 167, in test_thresholdsize
    self.assertRasterFitsUnivar(
  File "/home/mneteler/software/grass83/dist.x86_64-pc-linux-gnu/etc/python/grass/gunittest/case.py", line 301, in assertRasterFitsUnivar
    self.assertModuleKeyValue(
  File "/home/mneteler/software/grass83/dist.x86_64-pc-linux-gnu/etc/python/grass/gunittest/case.py", line 283, in assertModuleKeyValue
    self.fail(self._formatMessage(msg, stdMsg))
AssertionError: Basin values must be in the range [2, 12] 
r.univar map=test_basin percentile=90.0 nprocs=1 separator== -g difference:
mismatch values (key, reference, actual): [('max', 12, 4)]
command: r.univar map=test_basin percentile=90.0 nprocs=1 separator== -g {'map': 'test_basin', 'separator': '=', 'flags': 'g'}

(BTW: the issue was reported on the Italian GRASS GIS mailing list)

@neteler
Copy link
Member

neteler commented Sep 6, 2023

@wenzeslaus can we change the Centos7 CI runner to not required? Otherwise we'll not be able to merge this rather important and urgent bug fix...

@wenzeslaus
Copy link
Member

Having a failing main and all PRs is bad, too. I suggest reverting the change which caused that and/or removing the CentOS 7 check given how outdated it became and its low usefulness.

@neteler
Copy link
Member

neteler commented Sep 6, 2023

Having a failing main and all PRs is bad, too. I suggest reverting the change which caused that and/or

This is not an option as the old runner will go EOL on Sept 11, 2023.

removing the CentOS 7 check given how outdated it became and its low usefulness.

Done in #3141

@neteler
Copy link
Member

neteler commented Sep 7, 2023

PR #3141 (drop Centos7) has been merged, this PR needs to be rebased to become merge'able.

@metzm metzm merged commit b925233 into OSGeo:main Sep 11, 2023
18 checks passed
@metzm metzm deleted the watershed_basins branch September 11, 2023 09:00
metzm added a commit that referenced this pull request Sep 11, 2023
* r.watershed: fix streams and basins, improve testsuite
@metzm metzm modified the milestones: 8.4.0, 8.3.1 Sep 11, 2023
landam pushed a commit to landam/grass that referenced this pull request Oct 25, 2023
* r.watershed: fix streams and basins, improve testsuite
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
* r.watershed: fix streams and basins, improve testsuite
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C Related code is in C raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants