r.mapcalc neighbour modifier uses wrong row

[marisn]

Seems that in some occasions r.mapcalc neighbour modifier is using values from wrong row. A further analysis is required. Setting as a blocker as it causes silent result corruption (produces wrong results that could be mistaken for correct ones).

Steps to reproduce with NC:

g.region n=224480.25 s=223910.25 w=635623 e=636449.5 res=28.5
r.mapcalc "nd = if(landclass96[1,0]!=landclass96, landclass96[1,0], null())"

Visually examine both maps landclass96 and nd - values in "nd" should be only above borders present in landclass96 map. In one case, there are values below border (an attachment will be provided with markings for easier spotting).

First reported here: ​https://lists.osgeo.org/pipermail/grass-dev/2016-February/078918.html

Tested with 7.1 and: GRASS versija: 7.0.4svn GRASS SVN Revision: 67825 Būvējuma datums: 2016-01-14 Build Platform: x86_64-pc-linux-gnu GDAL/OGR: 2.0.1 PROJ.4: 4.8.0 GEOS: 3.5.0 SQLite: 3.10.2 Python: 2.7.11 wxPython: 3.0.2.0 Platform: Linux-4.4.0-gentoo-x86_64-Intel-R-_Core-TM-2_Duo_CPU_P9500_@_2.53GHz-with-gentoo-2.2

[marisn]

Marked cells should be located one row higher or to be completely absent

[glynn]

Replying to marisn:

Seems that in some occasions r.mapcalc neighbour modifier is using values from wrong row.

This appears to have been introduced in r34444 (Nov 2008), when the use of rowio was replaced with an internal cache.

It should be fixed by r67851.

[marisn]

I confirm based on the test case - issue is solved for 7.1. Now we have only a "small" problem - it has been present in all releases since 6.3.1 thus potentially compromising any computation based on it.

1) It must be backported to 7.x and 6.x

2) A clarification on potential impact should be provided to allow endusers to evaluate potentially affected results of any analysis performed with 6.3-7.0.

Replying to glynn:

Replying to marisn:

Seems that in some occasions r.mapcalc neighbour modifier is using values from wrong row.

This appears to have been introduced in r34444 (Nov 2008), when the use of rowio was replaced with an internal cache.

It should be fixed by r67851.

[mlennert]

Replying to marisn:

I confirm based on the test case - issue is solved for 7.1. Now we have only a "small" problem - it has been present in all releases since 6.3.1 thus potentially compromising any computation based on it.

1) It must be backported to 7.x and 6.x

I backported to release70. r67922

The issue is not present in 6.x.

2) A clarification on potential impact should be provided to allow endusers to evaluate potentially affected results of any analysis performed with 6.3-7.0.

What I don't really understand (without having taken the time to delve into the code) is why

r.mapcalc "nd = if(landclass96[1,0]!=landclass96, landclass96[1,0], null())"

provokes the bug, but

r.mapcalc "nd_temp = landclass96[1,0]"
r.mapcalc "nd2 = if(nd_temp!=landclass96, nd_temp, null())"

doesn't.

This would be important to understand in order to explain the impact. Also: where do want such an explanation ? A mail to grass-user and grass-dev ?

[glynn]

Replying to mlennert:

What I don't really understand (without having taken the time to delve into the code) is why

r.mapcalc "nd = if(landclass96[1,0]!=landclass96, landclass96[1,0], null())"

provokes the bug, but

r.mapcalc "nd_temp = landclass96[1,0]"
r.mapcalc "nd2 = if(nd_temp!=landclass96, nd_temp, null())"

doesn't.

In the first case, a single command uses two distinct row offsets, which causes the row cache to be used. In the second case, each command only uses a single row offset, so the row cache isn't used.

The precise test for whether the row cache is used is whether the number of cached rows is greater than one and less than or equal to 8. The number of cached rows is the maximum offset minus the minimum offset plus one. See setup_map() in raster/r.mapcalc/map.c.

[marisn]

Replying to mlennert:

I backported to release70. r67922

The issue is not present in 6.x.

Yes, sorry, I didn't check the code in careful way. Issue is present only in 7.0.0 release.

2) A clarification on potential impact should be provided to allow endusers to evaluate potentially affected results of any analysis performed with 6.3-7.0.

This would be important to understand in order to explain the impact. Also: where do want such an explanation ? A mail to grass-user and grass-dev ?

Looong time a go (Mon Nov 12 00:49:58 PST 2012) I made a proposal(1) on establishing guidelines how to handle situations when we discover code producing good looking but erroneous output (type II error) or corrupting existing data in other silent way. At the moment such issues are documented in issue tracker and intermixed with other discussions on -dev ML thus not providing an easy way for data quality issue tracking for outsiders (ordinary users). Probably it is necessary to revisit my idea and then decide to close this issue or not (as the problem with code has been fixed).

1. ​https://lists.osgeo.org/pipermail/grass-dev/2012-November/060780.html

[marisn]

Replying to glynn:

In the first case, a single command uses two distinct row offsets, which causes the row cache to be used. In the second case, each command only uses a single row offset, so the row cache isn't used.

The precise test for whether the row cache is used is whether the number of cached rows is greater than one and less than or equal to 8. The number of cached rows is the maximum offset minus the minimum offset plus one. See setup_map() in raster/r.mapcalc/map.c.

If I understood correctly, the issue was triggered by any mapcalc expression containing two to eight references to same map (MAP[n,x] and MAP[m,x], ...) where n!=m.

[pvanbosgeo]

Replying to marisn:

Replying to mlennert:

I backported to release70. r67922

The issue is not present in 6.x.

Yes, sorry, I didn't check the code in careful way. Issue is present only in 7.0.0 release.

2) A clarification on potential impact should be provided to allow endusers to evaluate potentially affected results of any analysis performed with 6.3-7.0.

This would be important to understand in order to explain the impact. Also: where do want such an explanation ? A mail to grass-user and grass-dev ?

Looong time a go (Mon Nov 12 00:49:58 PST 2012) I made a proposal(1) on establishing guidelines how to handle situations when we discover code producing good looking but erroneous output (type II error) or corrupting existing data in other silent way. At the moment such issues are documented in issue tracker and intermixed with other discussions on -dev ML thus not providing an easy way for data quality issue tracking for outsiders (ordinary users). Probably it is necessary to revisit my idea and then decide to close this issue or not (as the problem with code has been fixed).

1. ​https://lists.osgeo.org/pipermail/grass-dev/2012-November/060780.html

+1 providing a clear and easy way to find out about serious issue like above would certainly be a great step towards even more transparency and quality

[glynn]

Replying to marisn:

If I understood correctly, the issue was triggered by any mapcalc expression containing two to eight references to same map (MAP[n,x] and MAP[m,x], ...) where n!=m.

The row cache is contiguous, but limited to 8 rows. So it's used if the range of offsets (maximum row offset minus the minimum row offset plus one) is between 2 and 8 inclusive.

So if an expression contained e.g. map[-10] and map[10], the cache wouldn't be used (because it would need to hold 21 rows, which is more than the maximum of 8 rows). In that case, each row would be read multiple times rather than being cached.

Any expression using two or more distinct row offsets for a map (no offset is equivalent to an offset of zero) "might" trigger it. Scripts which use fixed offsets typically use small offsets, so the range being less than or equal to 8 is likely. Scripts which use larger row offsets typically do so by way of using variable row offsets, so it's likely that the cache will be used "sometimes".

[mlennert]

I would suggest that we close this bug as it is fixed. Maris, would you be willing to put your initial idea concerning communication about such bugs into an RFC on the ​trac wiki ? Then we can put it to a vote to the PSC.

[marisn]

Replying to mlennert:

Maris, would you be willing to put your initial idea concerning communication about such bugs into an RFC on the ​trac wiki ? Then we can put it to a vote to the PSC.

Done. I created a GRASS GIS errata framework proposal page: ​https://trac.osgeo.org/grass/wiki/RFC/5_Errata

Closing this issue as it is solved. Any discussion on communication with potentially affected users should be moved to -dev mailinglist.