[GRASS-dev] Different v.mkgrid table output depending on grid size

Michel_Wortmann1 · November 27, 2015, 10:14pm

Hi Devs,
I have been using the v.mkgrid command extensively recently and noticed that the tables of the output differ depending on the grid size. Apart from not having the columns rown and coln, larger grids’ row column counts from bottom to top rather than top to bottom as with smaller grids. I have dug in the code and found the appropriate lines in the main.c:

447 if (grid_info.num_rows < 27 && grid_info.num_cols < 27) {
448 sprintf(buf, “( %d, %d, %d, ‘%c’, ‘%c’ )”,
449 attCount + 1, grid_info.num_rows - i,
450 j + 1, ‘A’ + grid_info.num_rows - i - 1, ‘A’ + j);
451 }
452 else {
453 sprintf(buf, “( %d, %d, %d )”,
454 attCount + 1, i + 1, j + 1);

Has this a good reason or is this a bug? I guess inconsistent output is undesirable here.
Best,
Michel

marisn · November 28, 2015, 9:08am

I would call it a (undocumented) feature not a bug as it dates back to
initial revision of v.mkgrid.
Unfortunately I could not find the initial commit with a rationale
behind magical number 27. Earliest change I found was from 2004, when
Radim just commits v.mkgrid with message "upgrade". It is possible
that this feature(?) dates back to original version "Written by GRASS,
Fall of 88, Michael H." thus its original meaning seems to be lost.

I would suggest to go to trac and open a bug (with a patch,
preferably). Question, of course, is - which behaviour is the correct
one (count from bottom, count from top).

https://trac.osgeo.org/grass/changeset/13320

Māris.

2015-11-28 0:14 GMT+02:00 Michel Wortmann <wortmann@pik-potsdam.de>:

Hi Devs,
I have been using the v.mkgrid command extensively recently and noticed that
the tables of the output differ depending on the grid size. Apart from not
having the columns rown and coln, larger grids’ row column counts from
bottom to top rather than top to bottom as with smaller grids. I have dug in
the code and found the appropriate lines in the main.c:

447 if (grid_info.num_rows < 27 && grid_info.num_cols <
27) {
448 sprintf(buf, "( %d, %d, %d, '%c', '%c' )",
449 attCount + 1, grid_info.num_rows - i,
450 j + 1, 'A' + grid_info.num_rows - i - 1,
'A' + j);
451 }
452 else {
453 sprintf(buf, "( %d, %d, %d )",
454 attCount + 1, i + 1, j + 1);

Has this a good reason or is this a bug? I guess inconsistent output is
undesirable here.
Best,
Michel

_______________________________________________
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Michel_Wortmann1 · November 28, 2015, 10:24am

The magical number 27 comes from the fact that it puts letters into rown and coln and the alphabet is exhausted after 27 (although I wonder why the condition is && and not ||?). The inconsistency is just related to the counting of the row, i.e. the second argument to sprintf() in the else statement. So all thats needed is to replace the i + 1 with grid_info.num_rows - i.

Michel

On 28 Nov 2015, at 10:08, Maris Nartiss <maris.gis@gmail.com> wrote:

I would call it a (undocumented) feature not a bug as it dates back to
initial revision of v.mkgrid.
Unfortunately I could not find the initial commit with a rationale
behind magical number 27. Earliest change I found was from 2004, when
Radim just commits v.mkgrid with message “upgrade”. It is possible
that this feature(?) dates back to original version “Written by GRASS,
Fall of 88, Michael H.” thus its original meaning seems to be lost.

I would suggest to go to trac and open a bug (with a patch,
preferably). Question, of course, is - which behaviour is the correct
one (count from bottom, count from top).

https://trac.osgeo.org/grass/changeset/13320

Māris.

neteler · November 28, 2015, 5:15pm

On Sat, Nov 28, 2015 at 11:24 AM, Michel Wortmann
<wortmann@pik-potsdam.de> wrote:

The magical number 27 comes from the fact that it puts letters into rown and
coln and the alphabet is exhausted after 27 (although I wonder why the
condition is && and not ||?). The inconsistency is just related to the
counting of the row, i.e. the second argument to sprintf() in the else
statement. So all thats needed is to replace the i + 1 with
grid_info.num_rows - i.

Can you please send a diff?

Markus

Michel_Wortmann1 · December 2, 2015, 10:20pm

Hi Markus, sorry so late. Here is my diff of v.mkgrid/main.c

diff --git a/vector/v.mkgrid/main.c b/vector/v.mkgrid/main.c
index db733fd…5004fa0 100644
— a/vector/v.mkgrid/main.c
+++ b/vector/v.mkgrid/main.c
@@ -451,7 +451,7 @@ int main(int argc, char *argv)
}
else {
sprintf(buf, “( %d, %d, %d )”,

attCount + 1, i + 1, j + 1);

attCount + 1, grid_info.num_rows - i, j + 1);
}
if (db_append_string(&sql, buf) != DB_OK)
G_fatal_error(_(“Unable to fill attribute table”));

It’s just this single line that is changed.
Regards,
Michel

On 28 Nov 2015, at 18:15, Markus Neteler <neteler@osgeo.org> wrote:

On Sat, Nov 28, 2015 at 11:24 AM, Michel Wortmann
<wortmann@pik-potsdam.de> wrote:

The magical number 27 comes from the fact that it puts letters into rown and
coln and the alphabet is exhausted after 27 (although I wonder why the
condition is && and not ||?). The inconsistency is just related to the
counting of the row, i.e. the second argument to sprintf() in the else
statement. So all thats needed is to replace the i + 1 with
grid_info.num_rows - i.

Can you please send a diff?

Markus

neteler · December 27, 2015, 9:24pm

Hi Michel, Maris,

On Wed, Dec 2, 2015 at 11:20 PM, Michel Wortmann
<wortmann@pik-potsdam.de> wrote:

Hi Markus, sorry so late. Here is my diff of v.mkgrid/main.c

diff --git a/vector/v.mkgrid/main.c b/vector/v.mkgrid/main.c
index db733fd..5004fa0 100644
--- a/vector/v.mkgrid/main.c
+++ b/vector/v.mkgrid/main.c
@@ -451,7 +451,7 @@ int main(int argc, char *argv)
                    }
                    else {
                        sprintf(buf, "( %d, %d, %d )",
- attCount + 1, i + 1, j + 1);
+ attCount + 1, grid_info.num_rows - i, j + 1);
                    }
                    if (db_append_string(&sql, buf) != DB_OK)
                        G_fatal_error(_("Unable to fill attribute table"));

It’s just this single line that is changed.
Regards,
Michel

Sorry for slow - to avoid that it gets lost, could you please open a ticket at
https://trac.osgeo.org/grass/

I locally applied your change and the result doesn't differ from
before. Maybe I did too much backporting today and don't see it any
more

@Maris: or apply to SVN?

thanks
Markus

marisn · December 28, 2015, 11:10am

As nobody objected, I would say - go for it (push to trunk). Still -
do we need so many backports instead of pushing next release?

Thanks, Markus, for keeping head up on such patches,
Maris.

2015-12-27 23:24 GMT+02:00 Markus Neteler <neteler@osgeo.org>:

Hi Michel, Maris,

On Wed, Dec 2, 2015 at 11:20 PM, Michel Wortmann
<wortmann@pik-potsdam.de> wrote:

Hi Markus, sorry so late. Here is my diff of v.mkgrid/main.c

diff --git a/vector/v.mkgrid/main.c b/vector/v.mkgrid/main.c
index db733fd..5004fa0 100644
--- a/vector/v.mkgrid/main.c
+++ b/vector/v.mkgrid/main.c
@@ -451,7 +451,7 @@ int main(int argc, char *argv)
                    }
                    else {
                        sprintf(buf, "( %d, %d, %d )",
- attCount + 1, i + 1, j + 1);
+ attCount + 1, grid_info.num_rows - i, j + 1);
                    }
                    if (db_append_string(&sql, buf) != DB_OK)
                        G_fatal_error(_("Unable to fill attribute table"));

It’s just this single line that is changed.
Regards,
Michel

Sorry for slow - to avoid that it gets lost, could you please open a ticket at
https://trac.osgeo.org/grass/

I locally applied your change and the result doesn't differ from
before. Maybe I did too much backporting today and don't see it any
more

@Maris: or apply to SVN?

thanks
Markus

neteler · December 28, 2015, 11:19am

On Mon, Dec 28, 2015 at 12:10 PM, Maris Nartiss <maris.gis@gmail.com> wrote:

As nobody objected, I would say - go for it (push to trunk).

ok, done in r67403.

Still I wish to see a test case to understand the differences.

Still - do we need so many backports instead of pushing next release?

Well, I continue to backport bugfixes as much as I can to earlier
releases, say, to the last stable in any case and if tremendous, also
to G6.4.

Of course a new major release to happen in 2016!

Thanks, Markus, for keeping head up on such patches,
Maris.

cheers
Markus