[GRASS-dev] nasty bug in v.what.rast (CVS ~ 2 weeks ago)

GRASS GIS GRASS GIS developer
1

# setup region
g.region rast=elevation.dem

# create a point
echo "597843.375|4920255.640625" | v.in.ascii out=testing

# add table and make a column for loading elev into
v.db.addtable testing
v.db.addcol testing col='elev double'

# read elevation.dem at the point:
v.what.rast vect=testing rast=elevation.dem col=elev

##
## Ack! what does this mean?
##
WARNING: Raster type is integer and column type is float
WARNING: More points (2) of category 1, value set to 'NULL'
1 categories loaded from table
1 categories loaded from vector
0 categories from vector missing in table
1 duplicate categories in vector
1 records updated
0 update errors

## check result: missing the elevation.dem value
db.select testing
cat|elev
1|

... it looks like this error is being thrown from some kind of caching effort:

# from v.what.rast/main.c:

for (point = 0 ; point < point_cnt ; point++) {
  if ( cache[point].count > 1 ) {
      G_warning ( _("More points (%d) of category %d, value set to 'NULL'"),
               cache[point].count, cache[point].cat );
      dupl_cnt++;
  }

  /* category exist in DB ? */
  cex = (int *) bsearch((void *) &(cache[point].cat), catexst, select,
sizeof(int), srch_cat);
  if ( cex == NULL ) { /* cat does not exist in DB */
      norec_cnt++;
      G_warning(_("No record for category %d in table <%s>"),
          cache[point].cat, Fi->table );
      continue;
  }

I am using CVS from a couple weeks ago.. could this have been recently
introduced / fixed?

I will download the latest CVS and report back.

Cheers,

Dylan

--
Dylan Beaudette
Soil Resource Laboratory
http://casoilresource.lawr.ucdavis.edu/
University of California at Davis
530.754.7341

2

On Thursday 15 November 2007, Dylan Beaudette wrote:

# setup region
g.region rast=elevation.dem

# create a point
echo "597843.375|4920255.640625" | v.in.ascii out=testing

# add table and make a column for loading elev into
v.db.addtable testing
v.db.addcol testing col='elev double'

# read elevation.dem at the point:
v.what.rast vect=testing rast=elevation.dem col=elev

##
## Ack! what does this mean?
##
WARNING: Raster type is integer and column type is float
WARNING: More points (2) of category 1, value set to 'NULL'
1 categories loaded from table
1 categories loaded from vector
0 categories from vector missing in table
1 duplicate categories in vector
1 records updated
0 update errors

## check result: missing the elevation.dem value
db.select testing
cat|elev
1|

... it looks like this error is being thrown from some kind of caching
effort:

# from v.what.rast/main.c:

for (point = 0 ; point < point_cnt ; point++) {
  if ( cache[point].count > 1 ) {
      G_warning ( _("More points (%d) of category %d, value set to 'NULL'"),
               cache[point].count, cache[point].cat );
      dupl_cnt++;
  }

  /* category exist in DB ? */
  cex = (int *) bsearch((void *) &(cache[point].cat), catexst, select,
sizeof(int), srch_cat);
  if ( cex == NULL ) { /* cat does not exist in DB */
      norec_cnt++;
      G_warning(_("No record for category %d in table <%s>"),
          cache[point].cat, Fi->table );
      continue;
  }

I am using CVS from a couple weeks ago.. could this have been recently
introduced / fixed?

I will download the latest CVS and report back.

Cheers,

Dylan

I have just confirmed that this bug is present in today's CVS.

--
Dylan Beaudette
Soil Resource Laboratory
http://casoilresource.lawr.ucdavis.edu/
University of California at Davis
530.754.7341

3

On Thursday 15 November 2007, Dylan Beaudette wrote:

# setup region
g.region rast=elevation.dem

# create a point
echo "597843.375|4920255.640625" | v.in.ascii out=testing

# add table and make a column for loading elev into
v.db.addtable testing
v.db.addcol testing col='elev double'

# read elevation.dem at the point:
v.what.rast vect=testing rast=elevation.dem col=elev

##
## Ack! what does this mean?
##
WARNING: Raster type is integer and column type is float
WARNING: More points (2) of category 1, value set to 'NULL'
1 categories loaded from table
1 categories loaded from vector
0 categories from vector missing in table
1 duplicate categories in vector
1 records updated
0 update errors

## check result: missing the elevation.dem value
db.select testing
cat|elev
1|

... it looks like this error is being thrown from some kind of caching
effort:

# from v.what.rast/main.c:

for (point = 0 ; point < point_cnt ; point++) {
  if ( cache[point].count > 1 ) {
      G_warning ( _("More points (%d) of category %d, value set to 'NULL'"),
               cache[point].count, cache[point].cat );
      dupl_cnt++;
  }

  /* category exist in DB ? */
  cex = (int *) bsearch((void *) &(cache[point].cat), catexst, select,
sizeof(int), srch_cat);
  if ( cex == NULL ) { /* cat does not exist in DB */
      norec_cnt++;
      G_warning(_("No record for category %d in table <%s>"),
          cache[point].cat, Fi->table );
      continue;
  }

I am using CVS from a couple weeks ago.. could this have been recently
introduced / fixed?

I will download the latest CVS and report back.

Cheers,

Dylan

And looking at cvs log vector/v.what.rast/main.c

the problem was probably introduced near:

revision 1.28
date: 2007/10/22 15:23:41; author: carlos; state: Exp; lines: +3 -3
Standardization
----------------------------
revision 1.27
date: 2007/10/22 00:25:33; author: glynn; state: Exp; lines: +8 -15
Optimise duplicate elimination

.. I have another copy of GRASS 6.3, with a copy of main.c dated 2007-10-19,
which does not have this problem.

--
Dylan Beaudette
Soil Resource Laboratory
http://casoilresource.lawr.ucdavis.edu/
University of California at Davis
530.754.7341

4

On Thursday 15 November 2007, Dylan Beaudette wrote:

# setup region
g.region rast=elevation.dem

# create a point
echo "597843.375|4920255.640625" | v.in.ascii out=testing

# add table and make a column for loading elev into
v.db.addtable testing
v.db.addcol testing col='elev double'

# read elevation.dem at the point:
v.what.rast vect=testing rast=elevation.dem col=elev

##
## Ack! what does this mean?
##
WARNING: Raster type is integer and column type is float
WARNING: More points (2) of category 1, value set to 'NULL'
1 categories loaded from table
1 categories loaded from vector
0 categories from vector missing in table
1 duplicate categories in vector
1 records updated
0 update errors

## check result: missing the elevation.dem value
db.select testing
cat|elev
1|

... it looks like this error is being thrown from some kind of caching
effort:

# from v.what.rast/main.c:

for (point = 0 ; point < point_cnt ; point++) {
  if ( cache[point].count > 1 ) {
      G_warning ( _("More points (%d) of category %d, value set to 'NULL'"),
               cache[point].count, cache[point].cat );
      dupl_cnt++;
  }

  /* category exist in DB ? */
  cex = (int *) bsearch((void *) &(cache[point].cat), catexst, select,
sizeof(int), srch_cat);
  if ( cex == NULL ) { /* cat does not exist in DB */
      norec_cnt++;
      G_warning(_("No record for category %d in table <%s>"),
          cache[point].cat, Fi->table );
      continue;
  }

I am using CVS from a couple weeks ago.. could this have been recently
introduced / fixed?

I will download the latest CVS and report back.

Cheers,

Dylan

Sorry for the spam, it looks like the bug is related to this:

cvs diff -r 1.26 -r 1.27 main.c
Index: main.c

RCS file: /home/grass/grassrepository/grass6/vector/v.what.rast/main.c,v
retrieving revision 1.26
retrieving revision 1.27
diff -r1.26 -r1.27
221,235c221,228
< i = 1;
< while ( i < point_cnt ) {
< if ( cache[i].cat == cache[i-1].cat ) {
< cache[i-1].count++;
< for ( j = i; j < point_cnt - 1; j++ ) {
< cache[j].row = cache[j+1].row;
< cache[j].col = cache[j+1].col;
< cache[j].cat = cache[j+1].cat;
< cache[j].count = cache[j+1].count;
< }
< point_cnt--;
< continue;
< }
< i++;
< }
---

    for (i = j = 0; j < point_cnt; j++)
        if (cache[i].cat != cache[j].cat)
          cache[++i] = cache[j];
        else
            cache[i].count++;
    point_cnt = i + 1;

reverting to this version fixed the problem for me. I am not sure how to
interpret what the changes implemented from 1.26 -> 1.27 are doing, although
it looks like some kind of optimization. Glynn? :slight_smile:

--
Dylan Beaudette
Soil Resource Laboratory
http://casoilresource.lawr.ucdavis.edu/
University of California at Davis
530.754.7341

5

Dylan Beaudette wrote:

Sorry for the spam, it looks like the bug is related to this:

cvs diff -r 1.26 -r 1.27 main.c
Index: main.c

RCS file: /home/grass/grassrepository/grass6/vector/v.what.rast/main.c,v
retrieving revision 1.26
retrieving revision 1.27
diff -r1.26 -r1.27
221,235c221,228
< i = 1;
< while ( i < point_cnt ) {
< if ( cache[i].cat == cache[i-1].cat ) {
< cache[i-1].count++;
< for ( j = i; j < point_cnt - 1; j++ ) {
< cache[j].row = cache[j+1].row;
< cache[j].col = cache[j+1].col;
< cache[j].cat = cache[j+1].cat;
< cache[j].count = cache[j+1].count;
< }
< point_cnt--;
< continue;
< }
< i++;
< }
---
>
> for (i = j = 0; j < point_cnt; j++)
> if (cache[i].cat != cache[j].cat)
> cache[++i] = cache[j];
> else
> cache[i].count++;
> point_cnt = i + 1;
>

reverting to this version fixed the problem for me. I am not sure how to
interpret what the changes implemented from 1.26 -> 1.27 are doing, although
it looks like some kind of optimization. Glynn? :slight_smile:

Yes, it's an optimisation (from O(n^2) to O(n)), but with an
off-by-one error which effectively duplicates the first entry.

Try this:

--- vector/v.what.rast/main.c 22 Oct 2007 15:23:41 -0000 1.28
+++ vector/v.what.rast/main.c 16 Nov 2007 00:31:01 -0000
@@ -219,7 +219,7 @@

     G_debug(1, "Points are sorted, starting duplicate removal loop");

- for (i = j = 0; j < point_cnt; j++)
+ for (i = 0, j = 1; j < point_cnt; j++)
         if (cache[i].cat != cache[j].cat)
       cache[++i] = cache[j];
         else

Oddly enough, Markus actually tested this before it was committed, but
only for timing, not results (see "v.what.rast speedup" thread from
around Oct 22).

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

6

On Thursday 15 November 2007, Glynn Clements wrote:

Dylan Beaudette wrote:
> Sorry for the spam, it looks like the bug is related to this:
>
> cvs diff -r 1.26 -r 1.27 main.c
> Index: main.c
> ===================================================================
> RCS file: /home/grass/grassrepository/grass6/vector/v.what.rast/main.c,v
> retrieving revision 1.26
> retrieving revision 1.27
> diff -r1.26 -r1.27
> 221,235c221,228
> < i = 1;
> < while ( i < point_cnt ) {
> < if ( cache[i].cat == cache[i-1].cat ) {
> < cache[i-1].count++;
> < for ( j = i; j < point_cnt - 1; j++ ) {
> < cache[j].row = cache[j+1].row;
> < cache[j].col = cache[j+1].col;
> < cache[j].cat = cache[j+1].cat;
> < cache[j].count = cache[j+1].count;
> < }
> < point_cnt--;
> < continue;
> < }
> < i++;
> < }
> ---
>
> > for (i = j = 0; j < point_cnt; j++)
> > if (cache[i].cat != cache[j].cat)
> > cache[++i] = cache[j];
> > else
> > cache[i].count++;
> > point_cnt = i + 1;
>
> reverting to this version fixed the problem for me. I am not sure how to
> interpret what the changes implemented from 1.26 -> 1.27 are doing,
> although it looks like some kind of optimization. Glynn? :slight_smile:

Yes, it's an optimisation (from O(n^2) to O(n)), but with an
off-by-one error which effectively duplicates the first entry.

Try this:

--- vector/v.what.rast/main.c 22 Oct 2007 15:23:41 -0000 1.28
+++ vector/v.what.rast/main.c 16 Nov 2007 00:31:01 -0000
@@ -219,7 +219,7 @@

     G_debug(1, "Points are sorted, starting duplicate removal loop");

- for (i = j = 0; j < point_cnt; j++)
+ for (i = 0, j = 1; j < point_cnt; j++)
         if (cache[i].cat != cache[j].cat)
       cache[++i] = cache[j];
         else

Oddly enough, Markus actually tested this before it was committed, but
only for timing, not results (see "v.what.rast speedup" thread from
around Oct 22).

that did the trick. thanks.

That GRASS test suite is looking more and more important...

Dylan

--
Dylan Beaudette
Soil Resource Laboratory
http://casoilresource.lawr.ucdavis.edu/
University of California at Davis
530.754.7341