Skip to content

Comments

r.watershed: fix streams and basins#3140

Merged
metzm merged 2 commits intoOSGeo:mainfrom
metzm:watershed_basins
Sep 11, 2023
Merged

r.watershed: fix streams and basins#3140
metzm merged 2 commits intoOSGeo:mainfrom
metzm:watershed_basins

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
nilason
nilason approved these changes Sep 6, 2023
View reviewed changes
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

wenzeslaus commented Sep 6, 2023

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
@metzm metzm deleted the watershed_basins branch September 11, 2023 09:00
metzm added a commit that referenced this pull request Sep 11, 2023
@metzm
r.watershed: fix streams and basins (#3140)
05b017b 
* 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
@metzm @landam
r.watershed: fix streams and basins (OSGeo#3140)
8be7b39 
* r.watershed: fix streams and basins, improve testsuite
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
@metzm @neteler
r.watershed: fix streams and basins (OSGeo#3140)
d33d50f 
* 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

Reviewers

@nilason nilason nilason approved these changes

Assignees

No one assigned

Labels

bug Something isn't working C Related code is in C raster Related to raster data processing

Projects

None yet

Milestone

8.3.1

Development

Successfully merging this pull request may close these issues.

4 participants

@metzm @neteler @wenzeslaus @nilason