r.thin may be broken

[cmbarton]

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

[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

[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

[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

[cmbarton]

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

Michael

[cmbarton]

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

Michael

[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

[hamish]

ok, oh

[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

[cmbarton]

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

[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

[cmbarton]

I guess we can close this now.

Michael