[GRASS-dev] r.le--after some fixes still needs work

bakerwl1 · September 27, 2006, 7:18pm

Hello,

I tried r.le in CVS HEAD after Hamish made some fixes. The fixes took care of some problems. But, r.le.pixel still crashes with memory bug and r.le.setup does not display sampling units at all (communication with display window is broken). It will take some time to fix these. So, I do not think I can quickly get a working r.le. If you want a clean 6.2.0 release, then I suggest removing r.le from the next RC.

We can work on r.le and include it in 6.3.0 or just go forward with r.li. But, I am not opposed to leaving the r.le source code in the 6.2.0 release if you want to do that, even though it does not work correctly. The r.le.patch program “appears” to work OK–we could just leave it alone in the 6.20 release. It is r.le.setup and r.le.pixel that are broken in 6.2.0.

Bill Baker, Univ. of Wyoming

H_B · September 28, 2006, 8:17am

William L. Baker wrote:

I tried r.le in CVS HEAD after Hamish made some fixes. The fixes took
care of some problems.

..

But, r.le.pixel still crashes with memory bug

G63: spearfish> r.mapcalc elev=elevation.dem # it didn't like @othermapset
G63: spearfish> r.le.pixel elev att=b1

PARAMETER CHOICES:
        MAP: elev
        SAMPLE: whole map
        ATTRIBUTE MEASURES:
                  mean pixel attribute
mkdir: cannot create directory `r.le.out': File exists

R.LE.PIXEL IS WORKING....;

Segmentation fault

this happens in G_free(), raster/r.le/r.le.pixel/cellclip.c line 417:

  switch (data_type) {
     case CELL_TYPE:
        G_free (tmp);
        break;

compile time warnings:

cellclip.c: In function `cell_clip_drv':
cellclip.c:82: warning: passing arg 2 of `cell_clip' from incompatible pointer type
cellclip.c:129: warning: passing arg 2 of `center_is_not_null' from incompatible pointer type

and r.le.setup does not display sampling units at all (communication
with display window is broken).

Interactive modules need to call R_flush() after graphics calls if the
graphics driver is left open. (fixed in CVS) The mouse guide-box is
still not right, but I think that's minor- I'm more concerned with
getting the module functional.

It will take some time to fix these.

maybe just more simple little things...?

So, I do not think I can quickly get a working r.le. If you want a
clean 6.2.0 release, then I suggest removing r.le from the next RC.

we'll see how it goes over the next day or two, I await your feedback.
The documentation still needs porting from GRASS 5.4. (anyone keen?)

r.le.dist is something I should update to 6.XXX, but that will take
quite some time (months).

currently I only see the code in GRASS 4.3:
src.nonGPL/raster/r.le/r.le.dist/

so I guess it needs support for FCELL, DCELL maps (float/double);
NULL cells (r.mask); and of course some sort of GPL compatible
license.

Hamish

Glynn_Clements1 · September 28, 2006, 5:13pm

Hamish wrote:

> I tried r.le in CVS HEAD after Hamish made some fixes. The fixes took
> care of some problems.
..
> But, r.le.pixel still crashes with memory bug

G63: spearfish> r.mapcalc elev=elevation.dem # it didn't like @othermapset
G63: spearfish> r.le.pixel elev att=b1

PARAMETER CHOICES:
        MAP: elev
        SAMPLE: whole map
        ATTRIBUTE MEASURES:
                  mean pixel attribute
mkdir: cannot create directory `r.le.out': File exists

R.LE.PIXEL IS WORKING....;

Segmentation fault

this happens in G_free(), raster/r.le/r.le.pixel/cellclip.c line 417:

  switch (data_type) {
     case CELL_TYPE:
        G_free (tmp);
        break;

compile time warnings:

cellclip.c: In function `cell_clip_drv':
cellclip.c:82: warning: passing arg 2 of `cell_clip' from incompatible pointer type
cellclip.c:129: warning: passing arg 2 of `center_is_not_null' from incompatible pointer type

null_buf is being allocated with one char per column, but everything
else assumes that it holds one DCELL per column (although, AFAICT,
they only check for zero/non-zero).

The "right" solution is to change everything to treat it as char**;
the easy solution is to allocate one DCELL per column.

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

Brad_Douglas · September 28, 2006, 6:27pm

On Thu, 2006-09-28 at 18:13 +0100, Glynn Clements wrote:

Hamish wrote:

> > I tried r.le in CVS HEAD after Hamish made some fixes. The fixes took
> > care of some problems.
> ..
> > But, r.le.pixel still crashes with memory bug
>
> G63: spearfish> r.mapcalc elev=elevation.dem # it didn't like @othermapset
> G63: spearfish> r.le.pixel elev att=b1
>
> PARAMETER CHOICES:
> MAP: elev
> SAMPLE: whole map
> ATTRIBUTE MEASURES:
> mean pixel attribute
> mkdir: cannot create directory `r.le.out': File exists
>
> R.LE.PIXEL IS WORKING....;
>
> Segmentation fault
>
>
>
> this happens in G_free(), raster/r.le/r.le.pixel/cellclip.c line 417:
>
> switch (data_type) {
> case CELL_TYPE:
> G_free (tmp);
> break;
>
>
> compile time warnings:
>
> cellclip.c: In function `cell_clip_drv':
> cellclip.c:82: warning: passing arg 2 of `cell_clip' from incompatible pointer type
> cellclip.c:129: warning: passing arg 2 of `center_is_not_null' from incompatible pointer type

null_buf is being allocated with one char per column, but everything
else assumes that it holds one DCELL per column (although, AFAICT,
they only check for zero/non-zero).

The "right" solution is to change everything to treat it as char**;
the easy solution is to allocate one DCELL per column.

I got everything working last night, but there is one large issue: I
believe the modules needs segmentation. The largest region I have been
able to work with is about 150x150 with MAX defined as 100000. There is
no checking against MAX, so if you run out of allocated memory, it will
segv().

--
Brad Douglas <rez touchofmadness com> KB8UYR
Address: 37.493,-121.924 / WGS84 National Map Corps #TNMC-3785

bakerwl1 · September 28, 2006, 9:56pm

Hamish and Glynn, thank you for seeing that with null_buf. In r.le.patch, I did indeed allocate null_buf as DCELL, not char and that worked in there, even though it is not the right solution. Hamish, can you change that allocation statement for null_buf to DCELL and see if that fixes the crash? I’m not set up yet to do anything and can barely figure out CVS at this point. Hope that fixes the crash.

Hamish, as to r.le.setup, at the CHOOSE THE SETUP OPTION step, I choose Setup sampling units, then Use keyboard to enter sampling unit dimensions, then 1 for for How many different SCALES, then 1 for Random nonoverlapping, then “y” for Do you want to sample using rectangles, then “1.0” for Sampling unit SHAPE, then 144 for Recommended maximum SIZE followed by “y” it’s OK, then 20
sampling units. At that point, the program should draw the sampling units in red on the monitor, but nothing is displayed. Not sure where the communication problem is…

Bill
On Thu, 2006-09-28 at 18:13 +0100, Glynn Clements wrote:

Hamish wrote:

> > I tried r.le in CVS HEAD after Hamish made some fixes. The fixes took
> > care of some problems.
> ..
> > But, r.le.pixel still crashes with memory bug
> 
> G63: spearfish> r.mapcalc elev=elevation.dem # it didn't like @othermapset
> G63: spearfish> r.le.pixel elev att=b1
> 
> PARAMETER CHOICES:
>         MAP:      elev
>         SAMPLE:   whole map    
>         ATTRIBUTE MEASURES:
>                   mean pixel attribute
> mkdir: cannot create directory `r.le.out': File exists
> 
> R.LE.PIXEL IS WORKING....;
> 
> Segmentation fault
> 
> 
> 
> this happens in G_free(), raster/r.le/r.le.pixel/cellclip.c line 417:
> 
>   switch (data_type) {
>      case CELL_TYPE:
>         G_free (tmp);
>         break;
> 
> 
> compile time warnings:
> 
> cellclip.c: In function `cell_clip_drv':
> cellclip.c:82: warning: passing arg 2 of `cell_clip' from incompatible pointer type
> cellclip.c:129: warning: passing arg 2 of `center_is_not_null' from incompatible pointer type

null_buf is being allocated with one char per column, but everything
else assumes that it holds one DCELL per column (although, AFAICT,
they only check for zero/non-zero).

The "right" solution is to change everything to treat it as char**;
the easy solution is to allocate one DCELL per column.

H_B · September 29, 2006, 1:49am

William L. Baker wrote:

Hamish and Glynn, thank you for seeing that with null_buf. In
r.le.patch, I did indeed allocate null_buf as DCELL, not char and that
worked in there, even though it is not the right solution. Hamish, can
you change that allocation statement for null_buf to DCELL and see if
that fixes the crash?

Yes, it fixes it AFAICT. Applied in CVS. Another one down, thanks Glynn.

I'm not set up yet to do anything and can barely
figure out CVS at this point.

my 1 minute tutorial:

download latest 6.3-cvs snapshot
un-tar.gz

# connect to CVS server as read-only guest
$ export CVSROOT=:pserver:grass-guest@intevation.de:/home/grass/grassrepository
$ cvs login
pass: grass

#update to latest code
cvs up -dP

#make your changes
cd grass6/raster/r.le/r.le.pixel/
vi cellclip.c
make
# test
vi && make && etc..

#generate patch
cd ../../../..
cvs diff -u grass6/raster/r.le/r.le.pixel/cellclip.c > somepatch.diff

it's handy to make a ~/.cvsrc file. Mine contains:
-z3
diff -u
update -dP

Hamish, as to r.le.setup, at the CHOOSE THE SETUP OPTION step, I
choose Setup sampling units, then Use keyboard to enter sampling unit
dimensions, then 1 for for How many different SCALES, then 1 for
Random nonoverlapping, then "y" for Do you want to sample using
rectangles, then "1.0" for Sampling unit SHAPE, then 144 for
Recommended maximum SIZE followed by "y" it's OK, then 20
sampling units. At that point, the program should draw the sampling
units in red on the monitor, but nothing is displayed. Not sure where
the communication problem is...

Ah, ok I was trying with the mouse. If you look closely you'll see the
boxes are drawn right at the top of the window. i.e. the y values being
sent to draw_box() are no good. This is because y=0.0 when this calculation
is done:

raster/r.le/r.le.setup/sample, line ~ 878 in calc_unit_loc() back:

draw_box((int)((double)(ux[i])/x), (int)((double)(uy[i])/y),
(int)((double)(ux[i]+u_w)/x), (int)((double)(uy[i]+u_l)/y),
1);

after drawing the boxes (or attempting to anyway),
Distributed unit 20 of 20 requested
Is this set of sampling units OK? (y/n) [y] y
Segmentation fault

but one thing at a time.

Hamish

H_B · September 29, 2006, 1:57am

Oh & by the way, the reason that r.le.setup was hanging on this call:

G_system("d.frame -e")

was probably because the graphics driver was open when that call was
made, and d.frame can't run until the graphics driver is closed
again. So d.frame was both waiting for the driver to close and halting
r.le.setup in the middle, so the driver couldn't close.

Hamish

Glynn_Clements1 · September 29, 2006, 5:14pm

Hamish wrote:

Oh & by the way, the reason that r.le.setup was hanging on this call:

G_system("d.frame -e")

was probably because the graphics driver was open when that call was
made, and d.frame can't run until the graphics driver is closed
again. So d.frame was both waiting for the driver to close and halting
r.le.setup in the middle, so the driver couldn't close.

Note that the above call is essentially equivalent to:

Dscreen();

BTW, r.le.setup shouldn't be calling R_*/D_*/D* functions. Modules
which use the display are supposed to be called d.<something> (e.g.
d.le.setup), not r.<something>.

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

bakerwl1 · September 29, 2006, 5:47pm

Hamish, Glynn,

AFAICT r.le.pixel works fine now, as does r.le.patch.
Remaining problem just in r.le.setup. All of r.le.setup seems to work except Setup sampling units.

When setting up sampling units, you are right the problem is likely in calc_unit_loc in filling the uy array, which holds the y values for displaying the units on the screen. I have looked through that code and don’t see anything obvious that is wrong, so I will have to insert some print statements and see what is going on. Other displays of sampling areas on screen work fine (e.g., in setup for moving window). Seems strange, since this worked fine under GRASS 5.X and I can’t see what might have changed to affect this. Will work more on this in next few days, I hope.

Thanks for cvs tips!

Yes, Glynn is right this uses display, so this could be d.le.setup.

Bill B.

Hamish wrote:

Paolo_Cavallini · September 30, 2006, 6:29am

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

that's one of the reasons that promptet our investment in r.li. I
seriusly think your expertise and time will produce faster and better
output if you concentrate on r.li. Please note that r.li is fully
functional (it just does not have many modules implemented, but due to
its modularity this should be easy and fast to do).
Or there is a point I'm missing?
All the best.
pc

Brad Douglas ha scritto:

I got everything working last night, but there is one large issue: I
believe the modules needs segmentation. The largest region I have been
able to work with is about 150x150 with MAX defined as 100000. There is
no checking against MAX, so if you run out of allocated memory, it will
segv().

- --
Paolo Cavallini
email+jabber: cavallini@faunalia.it
www.faunalia.it
Piazza Garibaldi 5 - 56025 Pontedera (PI), Italy Tel: (+39)348-3801953
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFHg7P/NedwLUzIr4RAk98AJ4kPHVP0AzP3TOLgiIlOKy49uAUNwCfVU2E
cR9ICPz3s6EwLl+idUfgiiE=
=4mDL
-----END PGP SIGNATURE-----

bakerwl1 · September 30, 2006, 5:03pm

Hello Brad and Paolo,

It’s been a while since I looked at the code, but the MAX definition I think is for the number of categories allowed in r.le.pixel, not for the size the region. Unfortunately, we made a bad programming decision long ago. We should have used a linked-list structure for categories (which would have allowed any number of categories memory permits). Instead, MAX is used to allocate a fixed amount of memory for the categories structure. That worked fine when r.le.pixel and GRASS were categorical, but with floating point, there are huge numbers of categories possible. r.le.pixel will work on large regions (e.g., 10,000 X 10,0000 pixels) if there are only limited categories. But, to fix this to work really well for floating point will require replacement of the structure and associated coding for categories, so there is no fixed limit (MAX).

However, one could simply tell the user that it would be best to use r.le.pixel only with categorical data. It actually is of questionable scientific value to use it with floating point data, at least if there are large numbers of possible values. At one point I was going to add a warning message if the user wanted to analyze a floating point map with r.le.pixel–I think it is important that users understand that this module is not really meant for maps with very large numbers of values where the measures themselves would be of questionable value. I think this warning message approach is actually the right solution for now. But, in the long run, it would be nice to replace the structure with a linked list.

Paolo, I think there is some value to modularity, but in this case that is not the problem. It is our poor programming a long time ago!

Actually, I am not sure I understand why it really matters whether things are modular or not at the scale of individual measures? I think the one thing that could be said about r.le is that it is a bit old and the programming was not so great, so now we have to put some patches on the holes, rather than make something new that is better written. This case of MAX is a good example.

The chief advantage r.le had over FRAGSTATS and other programs not part of a GIS was r.le.setup, which allowed a diversity of sampling approaches, the moving window option (now FRAGSTATS has that), and integration within GRASS where output maps can be analyzed by other GIS programs. But, I still have not had the time to look at r.li!

Bill

Paolo Cavallini wrote:

Paolo_Cavallini · September 30, 2006, 6:16pm

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Agreed fully. These are the reasons why we started with r.li. Modularity
is only useful to include new indices as someone will find them useful.
A good thing is the client-server architecture and the parallel
computing. Please do find the time to have a look. I'm sure with your
contribution we'll get a good replacement for r.le in a short time.
Please do not hesitate posing questions (preferrably on the r.li mailing
list) if anything is not clear.
All the best.
pc

William L. Baker ha scritto:

The chief advantage r.le had over FRAGSTATS and other programs not part of a GIS
was r.le.setup, which allowed a diversity of sampling approaches, the moving
window option (now FRAGSTATS has that), and integration within GRASS where
output maps can be analyzed by other GIS programs. But, I still have not had the
time to look at r.li!

Bill

- --
Paolo Cavallini
email+jabber: cavallini@faunalia.it
www.faunalia.it
Piazza Garibaldi 5 - 56025 Pontedera (PI), Italy Tel: (+39)348-3801953
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFHrR0/NedwLUzIr4RArJ1AJ96c1pR9KfZeN9HrIZf3wkcZVPoLQCgnal2
5RW43oKkrWLzF2edcWJxOgk=
=5Hus
-----END PGP SIGNATURE-----

H_B · October 1, 2006, 5:09am

Glynn Clements wrote:

BTW, r.le.setup shouldn't be calling R_*/D_*/D* functions. Modules
which use the display are supposed to be called d.<something> (e.g.
d.le.setup), not r.<something>.

As a rule, probably not, but it does have 5+ years history using that
name and GRASS 6.0 was released with it being called that..

others modules that fall afoul: r.digit, v.digit, i.points, ...

Hamish

Brad_Douglas · October 1, 2006, 6:52am

On Sun, 2006-10-01 at 18:09 +1300, Hamish wrote:

Glynn Clements wrote:
> BTW, r.le.setup shouldn't be calling R_*/D_*/D* functions. Modules
> which use the display are supposed to be called d.<something> (e.g.
> d.le.setup), not r.<something>.

As a rule, probably not, but it does have 5+ years history using that
name and GRASS 6.0 was released with it being called that..

others modules that fall afoul: r.digit, v.digit, i.points, ...

This is why r.support is being rewritten as well...

--
Brad Douglas <rez touchofmadness com> KB8UYR
Address: 37.493,-121.924 / WGS84 National Map Corps #TNMC-3785

H_B · October 1, 2006, 8:21am

William wrote:

Hamish, as to r.le.setup, at the CHOOSE THE SETUP OPTION step, I choose
Setup sampling units, then Use keyboard to enter sampling unit
dimensions, then 1 for for How many different SCALES, then 1 for Random
nonoverlapping, then "y" for Do you want to sample using rectangles,
then "1.0" for Sampling unit SHAPE, then 144 for Recommended maximum
SIZE followed by "y" it's OK, then 20
sampling units. At that point, the program should draw the sampling
units in red on the monitor, but nothing is displayed. Not sure where
the communication problem is...

[...]

William L. Baker wrote:

Remaining problem just in r.le.setup. All of r.le.setup seems to work
except Setup sampling units.

When setting up sampling units, you are right the problem is likely in
calc_unit_loc in filling the uy array, which holds the y values for
displaying the units on the screen. I have looked through that code
and don't see anything obvious that is wrong, so I will have to insert
some print statements and see what is going on. Other displays of
sampling areas on screen work fine (e.g., in setup for moving window).
Seems strange, since this worked fine under GRASS 5.X and I can't see
what might have changed to affect this. Will work more on this in next
few days, I hope.

Hi, I had another look. in sample.c, mx[0], and mx[1] are not getting
passed correctly to the calc_unit_loc() fn.

this change makes it get a little further: (taken from the fn)

Index: setup.h

RCS file: /home/grass/grassrepository/grass6/raster/r.le/r.le.setup/setup.h,v
retrieving revision 2.1
diff -u -r2.1 setup.h
--- setup.h 9 Feb 2006 03:09:01 -0000 2.1
+++ setup.h 1 Oct 2006 08:10:46 -0000
@@ -78,7 +78,9 @@
void sample();
void man_unit();
void draw_grid();
-int calc_unit_loc();
+int calc_unit_loc(double, int, int, int, int, double, int, int, int,
+ double, int, int, int, double *, double *, int *,
+ double, int, int, double, double, double);
void get_rd();
void f();
int overlap();

these warnings are then given:

sample.c: In function `man_unit':
sample.c:564: warning: passing arg 14 of `calc_unit_loc' from incompatible pointer type
sample.c:564: warning: passing arg 15 of `calc_unit_loc' from incompatible pointer type

(that's ux and uy)

as it is called:

/* calculate the upper left corner of sampling
units and store them in arrays ux and uy */

        if (!calc_unit_loc(radius, t, b, l, r, ratio, u_w, u_l, method, intv, num, h_d,
           v_d, ux, uy, &sites, (int)(start[1]), (int)(start[0]), fmask, nx,
           mx[0], mx[1]))
           goto last;

and as defined in the fn:

/* CALCULATE THE COORDINATES OF THE TOP LEFT CORNER OF THE SAMPLING UNITS */
int calc_unit_loc (double radius, int top, int bot, int left, int right,
                   double ratio, int u_w, int u_l, int method, double intv,
                   int num, int h_d, int v_d, double *ux, double *uy,
                   int *sites, double startx, int starty, int fmask,
                   double nx, double x, double y)
{

Then, after the random quadrat boxes draw correctly, if you hit [y] you
still get a segfault, but again, one bug at a time.

Hamish