Opened 16 years ago
Closed 13 years ago
#96 closed defect (fixed)
v.surf.bspline column option broken
Reported by: | cmbarton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | 7.0.0 |
Component: | Raster | Version: | svn-releasebranch64 |
Keywords: | v.surf.bspline | Cc: | rantolin |
CPU: | All | Platform: | All |
Description
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.
Attachments (2)
Change History (35)
comment:1 by , 16 years ago
Milestone: | 6.3.0 → 6.4.0 |
---|
comment:2 by , 16 years ago
Component: | default → Raster |
---|---|
CPU: | → All |
Platform: | → All |
Summary: | v.surf.bspline intermittent failure - maybe on large datasets → v.surf.bspline broken |
comment:3 by , 16 years ago
Cc: | added |
---|
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
comment:5 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
This is fixed, but needs better documentation now--in progress.
comment:6 by , 15 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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
by , 15 years ago
Attachment: | v.surf.bspline_fix_attribs.diff added |
---|
bugfix proposal for use of attribute column
follow-up: 9 comment:7 by , 15 years ago
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
comment:8 by , 15 years ago
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
comment:9 by , 15 years ago
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.
comment:10 by , 15 years ago
Summary: | v.surf.bspline broken → v.surf.bspline column option broken |
---|
comment:11 by , 15 years ago
Keywords: | v.surf.bspline added |
---|
comment:12 by , 15 years ago
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
comment:13 by , 15 years ago
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
follow-up: 15 comment:14 by , 15 years ago
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
comment:15 by , 15 years ago
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
follow-ups: 18 20 22 comment:17 by , 15 years ago
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.
follow-up: 19 comment:18 by , 15 years ago
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.
comment:19 by , 15 years ago
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.
comment:20 by , 14 years ago
Milestone: | 6.4.0 → 7.0.0 |
---|---|
Version: | unspecified → svn-releasebranch64 |
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
follow-up: 23 comment:21 by , 14 years ago
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
comment:22 by , 14 years ago
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).
comment:23 by , 14 years ago
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?
comment:24 by , 14 years ago
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
follow-up: 26 comment:25 by , 14 years ago
(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)
comment:26 by , 14 years ago
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.
comment:27 by , 14 years ago
(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
follow-up: 29 comment:28 by , 14 years ago
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
follow-up: 30 comment:29 by , 14 years ago
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
comment:30 by , 14 years ago
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
comment:31 by , 14 years ago
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
comment:32 by , 14 years ago
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
comment:33 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
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