Decide if validity check in r.in.lidar should be an additional filter or a filter enabled by default

[wenzeslaus]

Based on discussion on ​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. ​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.

​Andrew Bell about PDAL on PDAL mailing list (see also ​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).

[wenzeslaus]

Patch to make point validity an additional filter

[mlennert]

Replying to wenzeslaus:

Based on discussion on ​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 ​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 ​FAQ)

[wenzeslaus]

Replying to 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 ​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).

[mlennert]

Replying to wenzeslaus:

Replying to 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.

[wenzeslaus]

In 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)

[wenzeslaus]

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

[wenzeslaus]

In 68959:

v.in.lidar, r.in.lidar and r3.in.lidar: make validity check an additional filter (backport r68951, see #3038)