[GRASS-dev] [GRASS GIS] #3038: Decide if validity check in r.in.lidar should be an additional filter or a filter enabled by default

GDAL · May 19, 2016, 9:49pm

#3038: Decide if validity check in r.in.lidar should be an additional filter or a
filter enabled by default
-------------------------------------------------+-------------------------
Reporter: wenzeslaus | Owner: grass-dev@…
Type: task | Status: new
Priority: normal | Milestone: 7.2.0
Component: Raster | Version: svn-trunk
Keywords: r.in.lidar, r3.in.lidar, | CPU: Unspecified
v.in.lidar, libLAS, PDAL |
Platform: Unspecified |
-------------------------------------------------+-------------------------
Based on discussion on [http://gis.stackexchange.com/a/187559/10759 GIS
SE] I wonder if we should make the filtering of lidar points based on
validity in r.in.lidar, r3.in.lidar, and v.in.lidar optional.

For us validity is defined by whatever `LASPoint_IsValid(LAS_point)`
considers valid. [http://www.liblas.org/utilities/las2las.html libLAS
documentation says]:

{{{
removes invalid (according to the ASPRS LAS file format specification)
points.
This switch should only be required in a few special circumstances. Points
that might be invalid include those with larger-than-required scan angles.
}}}

ASPRS specification mentions validity of angles and also some numbers but
otherwise I was not able to find clear information about valid and invalid
points.

PDAL does not have any concept like this and either all points are valid
or application itself must decide what is valid.

[https://lists.osgeo.org/pipermail/pdal/2015-September/000672.html Andrew
Bell about PDAL on PDAL mailing list] (see also
[http://www.pdal.io/tutorial/liblas_to_pdal.html#point-validity migration
guide]):

{{{
Point validity: there is no such concept
}}}

I've prepared a patch which turns the validity check into an additional
filter which needs to be explicitly enabled if user wants it. It also
removes validity check from scanning mode because no other filters are
applied at that point. ''This changes the current behavior, so the
question is if it is acceptable.''

The alternative is to switch the logic in the patch to disable the filter
if requested by user but have it on by default (i.e. current behavior by
default).

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3038>
GRASS GIS <https://grass.osgeo.org>

GDAL · May 19, 2016, 9:50pm

#3038: Decide if validity check in r.in.lidar should be an additional filter or a
filter enabled by default
-------------------------+-------------------------------------------------
  Reporter: wenzeslaus | Owner: grass-dev@…
      Type: task | Status: new
  Priority: normal | Milestone: 7.2.0
Component: Raster | Version: svn-trunk
Resolution: | Keywords: r.in.lidar, r3.in.lidar,
       CPU: | v.in.lidar, libLAS, PDAL
  Unspecified | Platform: Unspecified
-------------------------+-------------------------------------------------
Changes (by wenzeslaus):

* Attachment "inlidar_valid_points.diff" added.

Patch to make point validity an additional filter

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3038>
GRASS GIS <https://grass.osgeo.org>

GDAL · May 20, 2016, 8:04am

#3038: Decide if validity check in r.in.lidar should be an additional filter or a
filter enabled by default
-------------------------+-------------------------------------------------
  Reporter: wenzeslaus | Owner: grass-dev@…
      Type: task | Status: new
  Priority: normal | Milestone: 7.2.0
Component: Raster | Version: svn-trunk
Resolution: | Keywords: r.in.lidar, r3.in.lidar,
       CPU: | v.in.lidar, libLAS, PDAL
  Unspecified | Platform: Unspecified
-------------------------+-------------------------------------------------

Comment (by mlennert):

Replying to [ticket:3038 wenzeslaus]:
> Based on discussion on [http://gis.stackexchange.com/a/187559/10759 GIS
SE] I wonder if we should make the filtering of lidar points based on
validity in r.in.lidar, r3.in.lidar, and v.in.lidar optional.

[...]

> I've prepared a patch which turns the validity check into an additional
filter which needs to be explicitly enabled if user wants it. It also
removes validity check from scanning mode because no other filters are
applied at that point. ''This changes the current behavior, so the
question is if it is acceptable.''
>
> The alternative is to switch the logic in the patch to disable the
filter if requested by user but have it on by default (i.e. current
behavior by default).

I think that in line with the general rule of no API changes within the
same major number releases, the second option sounds better.

However, looking at the last comment in the SE exchange, it seems that
points are also declared invalid if the no scan angle is stored at all,
i.e. there are two different kinds of invalidity: those which are based on
a too large angle (and liblas seems to be quite restrictive, limiting this
to -90 - 90 - whereas more recent [http://www.asprs.org/wp-
content/uploads/2010/12/LAS_1_4_r13.pdf LAS specifications] allow -180 -
180).

So, I think we should:

* Add a flag to ignore validity checks
* Understand why scan angle 0 is declared invalid (if what Mihnea says on
SE is true) and solve that
* In the long run: determine whether laslib is still a good choice,
knowing that it seems to have stalled in development ("libLAS is in a
rather dormant period" as declared in the [http://www.liblas.org/faq.html
FAQ])

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3038#comment:1>
GRASS GIS <https://grass.osgeo.org>

GDAL · June 1, 2016, 2:53am

#3038: Decide if validity check in r.in.lidar should be an additional filter or a
filter enabled by default
-------------------------+-------------------------------------------------
  Reporter: wenzeslaus | Owner: grass-dev@…
      Type: task | Status: new
  Priority: normal | Milestone: 7.2.0
Component: Raster | Version: svn-trunk
Resolution: | Keywords: r.in.lidar, r3.in.lidar,
       CPU: | v.in.lidar, libLAS, PDAL
  Unspecified | Platform: Unspecified
-------------------------+-------------------------------------------------

Comment (by wenzeslaus):

Replying to [comment:1 mlennert]:
> ...the general rule of no API changes within the same major number
releases...

It is changing behavior because the output might be different. However, it
does not break scripts in general, although you might just get more points
or invalid points depending on your data. So. if it is an API break
depends on how much you care about invalid points in your data. The two
people on GIS SE didn't know about point validity and they actually want
all the points. I personally wouldn't know about validity if I didn't know
about the function call in the source code. (This is a call for anybody
who would be negatively affected by the change.)

Moreover, we don't promise validity G7:r.in.lidar filtering in the manual,
so the claim can be that invalid point filtering not behavior one should
rely upon.

> ...points are also declared invalid if the no scan angle is stored at
all, i.e. there are two different kinds of invalidity: those which are
based on a too large angle (and liblas seems to be quite restrictive,
limiting this to -90 - 90 - whereas more recent [http://www.asprs.org/wp-
content/uploads/2010/12/LAS_1_4_r13.pdf LAS specifications] allow -180 -
180).

The mess around it suggests that there is no clearly defined validity.
When validity itself is not clearly defined in standard and it changes for
individual properties with different format versions, what the library is
actually doing and how it behaves (will behave) with different format
versions? This seems to me like a buggy feature which should be disabled.

> * Add a flag to ignore validity checks

I still think that this change in the API is worth it. It might be even a
fix. So, I'm still for the opposite flag.

> * Understand why scan angle 0 is declared invalid ... and solve that

We can do some checks ourselves which would likely change the results/API
as well. But I don't know what to study anyway and if it is even worth it
as the practice may be really different from the theory here.

> * In the long run: determine whether libLAS is still a good choice...

Right. I think PDAL (which BTW doesn't have any validity check) is a good
replacement, but I wasn't able to touch it for some time (#2732).

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/3038#comment:2>
GRASS GIS <https://grass.osgeo.org>

GDAL · June 1, 2016, 1:13pm

#3038: Decide if validity check in r.in.lidar should be an additional filter or a
filter enabled by default
-------------------------+-------------------------------------------------
  Reporter: wenzeslaus | Owner: grass-dev@…
      Type: task | Status: new
  Priority: normal | Milestone: 7.2.0
Component: Raster | Version: svn-trunk
Resolution: | Keywords: r.in.lidar, r3.in.lidar,
       CPU: | v.in.lidar, libLAS, PDAL
  Unspecified | Platform: Unspecified
-------------------------+-------------------------------------------------

Comment (by mlennert):

Replying to [comment:2 wenzeslaus]:
> Replying to [comment:1 mlennert]:
> > ...the general rule of no API changes within the same major number
releases...
>
> It is changing behavior because the output might be different. However,
it does not break scripts in general, although you might just get more
points or invalid points depending on your data.

IMHO, the output counts as well, not only the fact that scripts might
break.

But as you say, in light of the very unclear definition of these points,
one could consider the silent suppression of points a bug, and so your
flag proposal a bug fix.

So, I would say: go ahead with your proposal (flag to filter non-valid
points), but maybe the module should output an information message about
this change.

> > * In the long run: determine whether libLAS is still a good choice...
>
> Right. I think PDAL (which BTW doesn't have any validity check) is a
good replacement, but I wasn't able to touch it for some time (#2732).

Yes, I guess PDAL would be a better choice than LASlib... It would be
great if there were a C-API to PDAL. I would be a bit hesitant to
introduce C++ dependence in what would probably be considered a core
module.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3038#comment:3>
GRASS GIS <https://grass.osgeo.org>

GDAL · July 12, 2016, 7:29pm

#3038: Decide if validity check in r.in.lidar should be an additional filter or a
filter enabled by default
-------------------------+-------------------------------------------------
  Reporter: wenzeslaus | Owner: grass-dev@…
      Type: task | Status: new
  Priority: normal | Milestone: 7.2.0
Component: Raster | Version: svn-trunk
Resolution: | Keywords: r.in.lidar, r3.in.lidar,
       CPU: | v.in.lidar, libLAS, PDAL
  Unspecified | Platform: Unspecified
-------------------------+-------------------------------------------------

Comment (by wenzeslaus):

In [changeset:"68951" 68951]:
{{{
#!CommitTicketReference repository="" revision="68951"
v.in.lidar, r.in.lidar and r3.in.lidar: make validity check an additional
filter but always warn about presence of invalid points (see #3038)
}}}

--
Ticket URL: </ticket/3038#comment:4>
GRASS GIS <https://grass.osgeo.org>

GDAL · July 14, 2016, 12:56pm

#3038: Decide if validity check in r.in.lidar should be an additional filter or a
filter enabled by default
-------------------------+-------------------------------------------------
  Reporter: wenzeslaus | Owner: grass-dev@…
      Type: task | Status: new
  Priority: normal | Milestone: 7.2.0
Component: Raster | Version: svn-trunk
Resolution: | Keywords: r.in.lidar, r3.in.lidar,
       CPU: | v.in.lidar, libLAS, PDAL
  Unspecified | Platform: Unspecified
-------------------------+-------------------------------------------------

Comment (by wenzeslaus):

Unless somebody is against that, I'll backport this (r68951) to 72.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3038#comment:5>
GRASS GIS <https://grass.osgeo.org>

GDAL · July 14, 2016, 6:18pm

#3038: Decide if validity check in r.in.lidar should be an additional filter or a
filter enabled by default
-------------------------+-------------------------------------------------
  Reporter: wenzeslaus | Owner: grass-dev@…
      Type: task | Status: new
  Priority: normal | Milestone: 7.2.0
Component: Raster | Version: svn-trunk
Resolution: | Keywords: r.in.lidar, r3.in.lidar,
       CPU: | v.in.lidar, libLAS, PDAL
  Unspecified | Platform: Unspecified
-------------------------+-------------------------------------------------

Comment (by wenzeslaus):

In [changeset:"68959" 68959]:
{{{
#!CommitTicketReference repository="" revision="68959"
v.in.lidar, r.in.lidar and r3.in.lidar: make validity check an additional
filter (backport r68951, see #3038)
}}}

--
Ticket URL: </ticket/3038#comment:6>
GRASS GIS <https://grass.osgeo.org>

GDAL · August 3, 2016, 12:24pm

#3038: Decide if validity check in r.in.lidar should be an additional filter or a
filter enabled by default
-------------------------+-------------------------------------------------
  Reporter: wenzeslaus | Owner: grass-dev@…
      Type: task | Status: closed
  Priority: normal | Milestone: 7.2.0
Component: Raster | Version: svn-trunk
Resolution: fixed | Keywords: r.in.lidar, r3.in.lidar,
       CPU: | v.in.lidar, libLAS, PDAL
  Unspecified | Platform: Unspecified
-------------------------+-------------------------------------------------
Changes (by wenzeslaus):

* status: new => closed
* resolution: => fixed

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/3038#comment:7>
GRASS GIS <https://grass.osgeo.org>