Opened 9 years ago

Closed 9 years ago

#1248 closed defect (fixed)

r.thin may be broken

Reported by: cmbarton Owned by: grass-dev@…
Priority: normal Milestone: 7.0.0
Component: Raster Version: svn-trunk
Keywords: r.thin Cc:
CPU: Unspecified Platform: Unspecified

Description

I recently tried to use r.thin in GRASS 7 (compiled from trunk a few days ago) and it seems to be behaving oddly. So I tested it against the Spearfish demo set. I ran r.watershed to generate a stream network (delete_streams) and then tried to thin it. The network generated by r.watershed is already down to a single cell in most places; only a few need thinning. But the r.thin output seems odd. It says it is deleting thousands of cells each iteration, with the number of cells deleted dropping by 8 each iteration. After 200 iterations, r.thin says that thinning is not completed. Looking at results, only a small fraction of the stream network was actually generated by r.thin. Here is an example of r.thin output:

r.thin input=delete_streams@PERMANENT output=delete_streams_thinned File delete_streams@PERMANENT -- 466 rows X 633 columns Bounding box: l = 2, r = 634, t = 2, b = 467 Pass number 1 Deleted 2194 pixels Pass number 2 Deleted 2186 pixels Pass number 3 Deleted 2178 pixels Pass number 4 Deleted 2170 pixels Pass number 5 ... Deleted 618 pixels Pass number 199 Deleted 610 pixels Pass number 200 Deleted 602 pixels Thinning not completed, consider to increase 'iterations' parameter. Output file 466 rows X 633 columns Window 466 rows X 633 columns

Change History (12)

comment:1 Changed 9 years ago by cmbarton

I'm trying to reformat the output so that it is easier to see.

r.thin input=delete_streams@PERMANENT output=delete_streams_thinned             
File delete_streams@PERMANENT -- 466 rows X 633 columns
Bounding box:  l = 2, r = 634, t = 2, b = 467
Pass number 1
Deleted 2194  pixels
Pass number 2
Deleted 2186  pixels
Pass number 3
Deleted 2178  pixels
Pass number 4
Deleted 2170  pixels
...
Pass number 197
Deleted 626  pixels
Pass number 198
Deleted 618  pixels
Pass number 199
Deleted 610  pixels
Pass number 200
Deleted 602  pixels
Thinning not completed, consider to increase 'iterations' parameter.
Output file 466 rows X 633 columns
Window 466 rows X 633 columns

comment:2 Changed 9 years ago by cmbarton

I just tested this in GRASS 6.4.1 and r.thin works fine. All thinning of the same map in spearfish was completed in 1 pass.

Michael

comment:3 in reply to:  2 Changed 9 years ago by mmetz

Replying to cmbarton:

I just tested this in GRASS 6.4.1 and r.thin works fine. All thinning of the same map in spearfish was completed in 1 pass.

Try r44645. The bug is that G_get_map_row() in 6.x converts NULLs to zeros, but there is no equivalent to G_get_map_row() in 7.0, where G_get_map_row() is usually replaced with Rast_get_c_row() which does not convert NULLs to zeros. The current fix converts NULLs to zeros in r.thin/io.c, but a proper fix would be to update r.thin and test against NULL values with !Rast_is_c_null_value() instead of e.g. buf[col] != 0. Padding should also use NULL and not zero. Therefore I would opt to change the status to enhancement if the current fix is working.

And all modules that use G_get_map_row() in 6.x should be checked...

Markus M

comment:4 Changed 9 years ago by cmbarton

I'll try the fix and let you know. Thanks.

Michael

comment:5 Changed 9 years ago by cmbarton

This seems to have fixed r.thin for now. Thanks.

Michael

comment:6 Changed 9 years ago by hamish

Resolution: fixed
Status: newclosed

comment:7 in reply to:  6 ; Changed 9 years ago by cmbarton

Replying to hamish:

Markus M suggested keeping this open, at least as an enhancement, because he didn't feel that this was a proper fix.

Michael

comment:8 Changed 9 years ago by hamish

Resolution: fixed
Status: closedreopened

ok, oh

comment:9 in reply to:  7 Changed 9 years ago by mmetz

Replying to cmbarton:

Replying to hamish:

Markus M suggested keeping this open, at least as an enhancement, because he didn't feel that this was a proper fix.

It should be properly fixed in r44866. Zero is now a valid number, all non-NULL cells are treated as lines.

Markus M

comment:10 Changed 9 years ago by cmbarton

AFAICT, this is fixed. Anyone want to try r.thin in GRASS 7 on another platform than Mac to be sure?

comment:11 in reply to:  10 Changed 9 years ago by mmetz

Replying to cmbarton:

AFAICT, this is fixed. Anyone want to try r.thin in GRASS 7 on another platform than Mac to be sure?

I tested on Linux before committing, works fine here. I would be really surprised if the changes, i.e. using Rast_is_c_null_value(), are not portable.

Markus M

comment:12 Changed 9 years ago by cmbarton

Resolution: fixed
Status: reopenedclosed

I guess we can close this now.

Michael

Note: See TracTickets for help on using tickets.