v.surf.bspline column option broken

[cmbarton]

v.surf.bspline is a potentially useful tool. But it seems to have bugs that show up intermittently. On modest size datasets, it works very fast. However, with many points, it seems to sit and do nothing. It's not clear whether it is locked up or becomes extremely slow. I'm still not sure if the column option works correctly.

[cmbarton]

This has set broken for many months now. Should we remove it from the main GRASS distribution until someone is able to work on it? It would be a nice addition to GRASS interpolation tools, but it doesn't seem like a good idea to ship a module that doesn't work and is not being fixed.

Michael

[neteler]

bugfix patch for testing from Roberto

[neteler]

I got a bugfix from Roberto which needs to be tested (patch attached). It includes G_percent() output and two message cosmetics from me.

Test case for North Carolina data set:

g.region res=500 vect=precip_30ynormals -ap
v.surf.bspline precip_30ynormals colum=annual rast=precip_30ynormals_surf layer=1 sin=1000 sie=1000

Markus

[martinl]

Can be the patch applied in SVN for better testing?

[cmbarton]

This is fixed, but needs better documentation now--in progress.

[neteler]

The bug is back... (interpolation of annual rainfall):

GRASS 6.5.svn (nc_spm_07):~ > g.region res=500 vect=precip_30ynormals -ap      
projection: 99 (Lambert Conformal Conic)                                       
zone:       0                                                                  
datum:      nad83                                                              
ellipsoid:  a=6378137 es=0.006694380022900787                                  
north:      307000                                                             
south:      27500                                                              
west:       151500                                                             
east:       917500                                                             
nsres:      500                                                                
ewres:      500                                                                
rows:       559                                                                
cols:       1532                                                               
cells:      856388                                                             

v.univar precip_30ynormals column=annual type=point
number of features with non NULL attribute: 136
number of missing attributes: 0
number of NULL attributes: 0
minimum: 947.42
maximum: 2329.18
range: 1381.76
mean: 1289.31
mean of absolute values: 1289.31
population standard deviation: 198.572
population variance: 39431
population coefficient of variation: 0.154014
sample standard deviation: 199.306
sample variance: 39723.1
kurtosis: 8.48418
skewness: 2.48352

v.surf.bspline precip_30ynormals rast=precip_30ynormals_surf column=annual layer=1 sin=1000 sie=1000
No vector map of sparse points to interpolate. Interpolation will be done with <precip_30ynormals> vector map
[136] records selected from table
 100%
v.surf.bspline complete.

r.univar precip_30ynormals_surf
 100%
total null and non-null cells: 856388
total null cells: 0

Of the non-null cells:
----------------------
n: 856388
minimum: 0
maximum: 0
range: 0
mean: 0
mean of absolute values: 0
standard deviation: 0
variance: 0
variation coefficient: nan %
sum: 0

[neteler]

bugfix proposal for use of attribute column

[neteler]

I found suspicious code which was introduced in the last fix round. Commenting that out attributes are taken again. I don't understand the purpose of the part which I have commented in attached patch v.surf.bspline_fix_attribs.diff. AFAIK "type" is defined but not set. There is another "type" in main() - perhaps a different variable was supposed to be used to test against GV_POINTS?

Markus

[neteler]

With that patch, the result of the precip vector map interpolation is now:

r.univar precip_30ynormals_surf
 100%
total null and non-null cells: 54144
total null cells: 0

Of the non-null cells:
----------------------
n: 54144
minimum: 1133.44
maximum: 1858.02
range: 724.578
mean: 1338.21
mean of absolute values: 1338.21
standard deviation: 153.479
variance: 23555.8
variation coefficient: 11.469 %
sum: 72456125.5301044583

which has a reasonable range. Artefacts are visible but that will require parameter tuning.

Markus

[marisn]

Replying to neteler:

I found suspicious code which was introduced in the last fix round. Commenting that out attributes are taken again. I don't understand the purpose of the part which I have commented in attached patch v.surf.bspline_fix_attribs.diff. AFAIK "type" is defined but not set. There is another "type" in main() - perhaps a different variable was supposed to be used to test against GV_POINTS?

Markus

Original code (almost) makes sense (r21982).

/*type = Vect_read_line (&In, points, Cats, observ[i].lineID);*/ 
if ( !(type & GV_POINTS ) ) continue; 

Still I have no idea how to fix this module (I'm lazy) (fix == does it needs that check or not). Also I looked at P_Read_Vector_Region_Map (lidarlib/zones.c) and it seemed somehow fishy - it reads vector line and then uses only first coordinate of that line, still as code lacks any comments about it's design specifics (and it has them i.e. as it crunches over all vector lines on every function call instad of using standart V_* functions) I can only suggest to somebody who understands that code to add comments and, probably, line type check.

[cmbarton]

I was just able to test this with an sqlite attribute database. The column option is still broken in all versions. If this is non-functional and no way to fix it, perhaps we should just take it out of the interface and manual.

Michael

[cmbarton]

Forgot to give an example of the command I used--that did not work.

Michael

v.surf.bspline --overwrite input=elev_pts1000_sql@sqlite_test raster=aaa_splintest@sqlite_test sie=400 sin=400 layer=1 column=elevation

[cmbarton]

I just tried out the updates in GRASS 7 trunk. The column option still doesn't work right. v.surf.bspline runs without complaining if you specify a column, but the output is a flat map that does not use the column values for interpolation.

Also, the layer selector no longer gives a -1 option in the GUI. So you cannot use a 3D map either.

Michael

[neteler]

Replying to cmbarton:

I just tried out the updates in GRASS 7 trunk. The column option still doesn't work right.

To my knowledge a "make distclean" is needed here.

Markus

[cmbarton]

I always do a make distclean. So this is not the problem.

Michael

[cmbarton]

Following Helena's suggestion, I did a make distclean BEFORE running svn up. Still same result. Cannot use 3D points and 2D points with attribute don't work.

So v.surf.bspline has gone for me from working under constrained conditions to not working at all.

BTW, using the layer=-1 for 3D points is poor interface design. There should be a flag for this. It should not depend on using an illegal value for layers.

A couple new results that I'll report elsewhere. The GUI no longer launches automatically and the r.bilinear is greyed out.

[hamish]

Replying to cmbarton:

BTW, using the layer=-1 for 3D points is poor interface design. There should be a flag for this. It should not depend on using an illegal value for layers.

well, if it helps, think of it as "layer=none" in a system constrained to integers. maybe not ideal, but not totally evil either. A programmer's trick which has snuck into the UI..

Hamish

ps- something weird with server reboots, ldap, or something, I'm having to re-logon into trac every hour or so the last couple of days.

[cmbarton]

Replying to hamish:

Replying to cmbarton:

BTW, using the layer=-1 for 3D points is poor interface design. There should be a flag for this. It should not depend on using an illegal value for layers.

well, if it helps, think of it as "layer=none" in a system constrained to integers. maybe not ideal, but not totally evil either. A programmer's trick which has snuck into the UI..

Yeah. I know. But it wreaks havoc with trying to make an autogenerated GUI. separate functions need to have separate arguments/flags.

Michael

Hamish

ps- something weird with server reboots, ldap, or something, I'm having to re-logon into trac every hour or so the last couple of days.

[mmetz]

Replying to cmbarton:

Cannot use 3D points and 2D points with attribute don't work.

That's a bug in the wxGUI, works fine from the command line.

BTW, using the layer=-1 for 3D points is poor interface design. There should be a flag for this. It should not depend on using an illegal value for layers.

I don't think it is up to the GUI to define what is an illegal value. If at all, it should use the module's definition of legal and illegal values and not come up with its own definitions. Many modules accept -1 as a legal value for the layer option, meaning all layers. Another option would be to leave the layer option empty (don't specify it) which requires that it must be possible to clear the layer option in the wxGUI.

The GUI no longer launches automatically and the r.bilinear is greyed out.

The wxGUI needs to be updated and the r.bilinear entry removed for grass7. This module has been merged into r.resamp.interp (see r.bilinear manual in grass64).

worksforme,

Markus M

[cmbarton]

The GUI is not defining what is an illegal value. -1 is not a valid value for a layer. There is no layer -1. Using 3D points instead of a column in an attribute table has nothing to do with picking layers. It is a choice of using a value from the attribute table OR using the z value of 3D points.

It is easy for the GUI to create a spin box for layer selection that runs from -1 to a max. But that would be the case for all interfaces that use layers. What happens with d.vect layer=-1?

The problem is using the layer selection to make the module do something that has nothing to do with layer selection. For ease of parsing in scripts and for consistent creation of a GUI interface, each distinct function should have it's own argument (option or flag). Trying to combine two different functions into one argument leads to problems in using the module syntax for anything beyond human command-line use.

I guess I don't see what the problem is in having a z flag.

BTW, I see that Martin has already fixed the r.bilinear entry a day or two ago.

Michael

[mmetz]

Replying to cmbarton:

So v.surf.bspline has gone for me from working under constrained conditions to not working at all.

Please post the exact commands you used. BTW, when using 3D vector points, just don't specify the layer option (it's optional anyway).

There are two vectors in the nc_spm_08 dataset that are suitable for testing the column and 3D points mode:

precip_30ynormals@PERMANENT

and

precip_30ynormals_3d@PERMANENT

A good spline step value for these vectors is 50000 (the default is meant for LiDAR points with 1m spacing).

[mmetz]

Replying to cmbarton:

It is easy for the GUI to create a spin box for layer selection that runs from -1 to a max. But that would be the case for all interfaces that use layers. What happens with d.vect layer=-1?

In grass7, nearly all modules have a layer option because of direct OGR read access added by Martin. Layer=-1 means, use all layers, as for d.vect. For most modules, the layer option is optional, i.e. it must be possible in the GUI to either leave it empty or specify a layer if necessary.

The problem is using the layer selection to make the module do something that has nothing to do with layer selection. For ease of parsing in scripts and for consistent creation of a GUI interface, each distinct function should have it's own argument (option or flag). Trying to combine two different functions into one argument leads to problems in using the module syntax for anything beyond human command-line use.

I agree, there should be a possibility to choose where the z values are coming from.

I guess I don't see what the problem is in having a z flag.

Added in trunk r41098, please test. Works for me in nc_spm_08 with both precip_30ynormals@PERMANENT and precip_30ynormals_3d@PERMANENT. When using z coordinates of precip_30ynormals_3D@PERMANENT, be aware that these are elevation, not precipitation.

If it passes your testing, backport to all grass6 branches?

[hamish]

backport to all grass6 branches?

my vote is no, at least not to 6.4 .. critical bugfixes only at this point please!

• if one of the guis is broken, fix the gui, don't rework the modules to work around deficiencies in something else.
• in grass7 bring v.surf.bspline and v.surf.rst's usage into sync.
• this module existed in 6.2 so layer=-1 must remain at the module level for backwards compatibility in future 6.x, and can be explained as a tooltip.

I wonder how we could allow string input there for layer="all" and layer="none" without ruining the built-in integer checks?

Hamish

[hamish]

(this module being designed for massive LIDAR point datasets, where there is no DB layer & attribute table, and probably no topology built either FWTW. that's not to say the module shouldn't be useful for other tasks, but that's what the module designers were thinking about)

[mmetz]

Replying to hamish:

(this module being designed for massive LIDAR point datasets, where there is no DB layer & attribute table, and probably no topology built either FWTW. that's not to say the module shouldn't be useful for other tasks, but that's what the module designers were thinking about)

Yes, most probably the module was designed to interpolate a DTM that is the result of the whole LiDAR processing procedure. But the module also works with z values taken from an attribute table (quite nice AFAICT), no need for topology here.

The layer option problem is IMHO purely a problem of the GUI because it must be possible to not give a layer number (layer option is optional), and if a layer number is given, that number must be given by the GUI to the module. As for other options expecting an integer value.

[hamish]

(to be honest I'm not really against the new -z flag at all, I'm just a bit frustrated about any new changes to the 6.4.0 branch when we should be deep in release-mode and I'm venting it here)

maybe have the wxGUI builder check to see if layer= is manadatory before deciding to do a dropdown list of valid layers?

Hamish

[cmbarton]

The problem is that the module has been in the distribution but has been completely broken for a long time. Only recently was it partly fixed so that it worked with 3D points. But using an attribute value for the interpolation remained broken. At that time, one picked layer=0 to use 3D points. AFAIC, this is a problematic design for reasons I outlined previously. BUT it did work in the GUI and command line.

Very recently there has been a commendable attempt to fix the attribute value bug. But along with this, for some reason, the way to use 3D points was changed from layer=0 (working in GUI) to layer=-1 (not working in GUI). And I was not able to get an interpolated map from the attribute either. I hope that this all is fixed and will recompile and test as soon as possible. But I'm in the middle of an NSF workshop right now and can't do it for a couple of days.

Michael

[mmetz]

Replying to cmbarton:

The problem is that the module has been in the distribution but has been completely broken for a long time. Only recently was it partly fixed so that it worked with 3D points. But using an attribute value for the interpolation remained broken. At that time, one picked layer=0 to use 3D points.

That is still true for v.surf.rst and v.surf.idw in grass64. I just discovered that in trunk, these modules got a new -z flag. v.surf.bspline should now be in sync with the other v.surf. modules in trunk. BTW, there is a mix for v.surf.rst in devbr6: -z flag and "If [layer is] set to 0, z coordinates are used. (3D vector only)". v.surf.idw doesn't have a -z flag in devbr6. A bit of a mess.

Very recently there has been a commendable attempt to fix the attribute value bug. But along with this, for some reason, the way to use 3D points was changed from layer=0 (working in GUI) to layer=-1 (not working in GUI).

Reversed for grass6.x, synced to the other v.surf.* modules, affected the wxGUI only, not tcltk. Works for me now with all GUIs and from the command line in all branches, both z coords and attribute column.

Markus M

[hamish]

Replying to mmetz:

Works for me now with all GUIs and from the command line in all branches, both z coords and attribute column.

... thanks guys

[cmbarton]

Thanks very much! I hope to recompile and test this weekend.

I just discovered a couple of other errors in my week-old version of GRASS 7 that I need to check on. Broken profiler and data manager. Hopefully these are already fixed in the svn.

Michael

[cmbarton]

I just tested this on a new version of GRASS 7 updated last night. It all seems to work great. Attribute interpolation works, z value interpolation works. I especially like the automatic estimation of interpoint distance to use for interpolation.

This suggests an enhancement for a future version. Option to run with automatic sie and sin based on this interpoint distance.

Thanks again for getting this important module working well.

I realize that it was initially design for dense lidar data sets. But it is a very powerful and very fast interpolation routine. I've found it quite useful for many task for which the other interpolators available do not work as well.

Michael