[GRASS-dev] [bug #4522] (grass) r.series: error in row of input map is non-fatal giving wrong results

this bug's URL: http://intevation.de/rt/webrt?serial_num=4522
-------------------------------------------------------------------------

Subject: r.series: error in row of input map is non-fatal giving wrong results

Hi,

a problem with r.series:

One of my input maps has a corruption in it (disk flaw??), using r.series
(method=average) I see this error:

WARNING: error reading compressed map [dbt_corrupt] in mapset
         [sw2006f_icw], row 1465

but the process continues to completion and I get a resulting map which looks
ok, but is subtly wrong.

G61> r.univar dbt_mean_corrupted
100%
total null and non-null cells: 11160000
total null cells: 9626502

Of the non-null cells:
----------------------
n: 1533498
minimum: 0
maximum: 1.8731
range: 1.8731
mean: 0.810907
standard deviation: 0.361051
variance: 0.130358
variation coefficient: 44.5243 %
sum: 1243524.7052483591

G61> r.univar dbt_mean_good
100%
total null and non-null cells: 11160000
total null cells: 9629925

Of the non-null cells:
----------------------
n: 1530075
minimum: 0.2611
maximum: 1.8731
range: 1.612
mean: 0.812793
standard deviation: 0.359435
variance: 0.129194
variation coefficient: 44.2223 %
sum: 1243634.0932001742

As this was called from a script it is lucky I noticed..

The read error should trigger a G_fatal_error().

thanks,
Hamish

-------------------------------------------- Managed by Request Tracker

Request Tracker wrote:

this bug's URL: http://intevation.de/rt/webrt?serial_num=4522
-------------------------------------------------------------------------

Subject: r.series: error in row of input map is non-fatal giving wrong results

Hi,

a problem with r.series:

One of my input maps has a corruption in it (disk flaw??), using r.series
(method=average) I see this error:

WARNING: error reading compressed map [dbt_corrupt] in mapset
         [sw2006f_icw], row 1465

but the process continues to completion and I get a resulting map which looks
ok, but is subtly wrong.

The read error should trigger a G_fatal_error().

Agreed. The question is: why does libgis leave this to individual
modules rather than doing it itself? Are there really modules which
might want to do something other than call G_fatal_error() in this
case?

FWIW, it isn't just r.series; the following all call G_get_raster_row
(and similar) without checking the return value:

display/d.rast.arrow
display/d.rast.edit
display/d.rast.num
display/d.rast
imagery/i.ifft
imagery/i.pca
lib/D
lib/ogsf
lib/rst/interp_float
paint/p.out.vrml
ps/ps.map
raster/r.bilinear
raster/r.carve
raster/r.contour
raster/r.flow
raster/r.grow2
raster/r.lake
raster/r.le/r.le.patch
raster/r.le/r.le.pixel
raster/r.le/r.le.setup
raster/r.le/r.le.trace
raster/r.neighbors
raster/r.param.scale
raster/r.random.cells
raster/r.random.surface
raster/r.recode
raster/r.series
raster/r.slope.aspect
raster/r.sum
raster/r.sun
raster/r.sunmask
raster/r.surf.area
raster/r.surf.contour
raster/r.texture
raster/r.to.vect
raster/r.volume
raster/r.water.outlet
raster/r.watershed/ram
raster/r.watershed/seg
raster/r.watershed/shed
raster/simwe/simlib
vector/v.vol.rst

--
Glynn Clements <glynn@gclements.plus.com>

> The read error should trigger a G_fatal_error().

Agreed. The question is: why does libgis leave this to individual
modules rather than doing it itself? Are there really modules which
might want to do something other than call G_fatal_error() in this
case?

I doubt it.

Is there a possibility of many orphaned .tmp files if the module exits
during a read while an output map open? i.e. how important is it to exit
with clean-up at any given point of a typical raster [read; do stuff;
write] operation?

Hamish

Hamish wrote:

> > The read error should trigger a G_fatal_error().
>
> Agreed. The question is: why does libgis leave this to individual
> modules rather than doing it itself? Are there really modules which
> might want to do something other than call G_fatal_error() in this
> case?

I doubt it.

Is there a possibility of many orphaned .tmp files if the module exits
during a read while an output map open?

Yes.

i.e. how important is it to exit
with clean-up at any given point of a typical raster [read; do stuff;
write] operation?

Almost nothing does this at present. Even modules which check the
return value from G_get_raster_row() etc usually just call
G_fatal_error() if it fails.

There are precisely 20 files in 15 modules which call G_unopen_cell(),
while there are rather more than that which create new raster maps.
The remainder simply don't bother to clean up upon errors (including
r.example).

I suggest that:

a) G_get_raster_row() etc should automatically call G_fatal_error() if
an error occurs, with a function to disable this behaviour for modules
which really need to do their own error handling.

b) It should be the library's responsibility to clean up incomplete
maps upon exit.

Having to manually check for error conditions and perform clean-up
whenever an error occurs is enough of a nuisance that it will
frequently be omitted (or, if error handling is added, those code
paths will seldom get tested and thus will frequently contain bugs).

--
Glynn Clements <glynn@gclements.plus.com>

I suggest that:

a) G_get_raster_row() etc should automatically call G_fatal_error() if
an error occurs, with a function to disable this behaviour for modules
which really need to do their own error handling.

ok

b) It should be the library's responsibility to clean up incomplete
maps upon exit.

Having to manually check for error conditions and perform clean-up
whenever an error occurs is enough of a nuisance that it will
frequently be omitted (or, if error handling is added, those code
paths will seldom get tested and thus will frequently contain bugs).

does that mean you have to start sending the file descriptors, etc,
along to the lib fns? While one read error probably means many, waiting
for cleanup-on-GRASS-exit seems less messy somehow.

Should G_get_raster_row() do system("$GISBASE/etc/clean_temp") before
calling G_fatal_error() ?

Hamish

Hamish wrote:

> a) G_get_raster_row() etc should automatically call G_fatal_error() if
> an error occurs, with a function to disable this behaviour for modules
> which really need to do their own error handling.

ok

> b) It should be the library's responsibility to clean up incomplete
> maps upon exit.
>
> Having to manually check for error conditions and perform clean-up
> whenever an error occurs is enough of a nuisance that it will
> frequently be omitted (or, if error handling is added, those code
> paths will seldom get tested and thus will frequently contain bugs).

does that mean you have to start sending the file descriptors, etc,
along to the lib fns? While one read error probably means many, waiting
for cleanup-on-GRASS-exit seems less messy somehow.

Should G_get_raster_row() do system("$GISBASE/etc/clean_temp") before
calling G_fatal_error() ?

My comment was limited to cleaning up the temporary cell/fcell file
created when writing new raster maps. Essentially, G_open_raster_new()
etc would need to use atexit() to automatically "unopen" any
incomplete maps.

Applications would be responsible for cleaning up their own temporary
files.

--
Glynn Clements <glynn@gclements.plus.com>