Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grid_canopy(p2r()) output size depends on subcircle tweak / breaks with res=raster #518

Closed
Hannes-Ole opened this issue Dec 9, 2021 · 2 comments
Assignees
Labels
Enhancement Not actually a bug but a possible improvement

Comments

@Hannes-Ole
Copy link

When using grid_canopy(), the output is dependend on the subcircle tweak used in p2r(). This makes sense, since after "expanding" each point to a disk, the extent of the point cloud is larger - it can be argued that it makes sense, that the user gets as chm the full extent of the "extended" point cloud - although having a larger output extent than input extent may still be surprising to some.

However the algorithm breaks when using a reference raster to determine alignment/resolution, and I think that shouldn't happen (I'd rather get the "cropped" chm). Here is a code example, if it isn't reproducible on your data I can send you some:

# create reference raster
ref_raster <- raster(extent(tile_dtm), res = 1)
crs(ref_raster) = crs(tile_dtm)

# make sure the las cloud extent is the same as the reference raster
tile_las = clip_roi(tile_las, extent(ref_raster))

Calling the following causes the error:

>   r_chm_bottom = grid_canopy(tile_las, res=ref_raster, p2r(0.5))
Fehler in C_rasterize(las, layout, subcircle, 1L) : 
  C++ unexpected internal error in 'rasterize': point out of raster.

Here we see, what the algorithm would like to output, without being restricted by an extent:

>   r_chm = grid_canopy(tile_las, res=1, p2r(0.5))
>   extent(r_chm_bottom)
class      : Extent 
xmin       : 2692999 
xmax       : 2694001 
ymin       : 1124999 
ymax       : 1126001 

When using grid_metrics or setting p2r(subcircle = 0) , which is the default, it works without complaints. A side question: Is grid_metrics(~max(Z)) basically equivalent to grid_canopy(p2r())?

>   r_npoints_all = grid_metrics(tile_las, ~max(Z), res=ref_raster)
>   extent(r_npoints_all)
class      : Extent 
xmin       : 2693000 
xmax       : 2694000 
ymin       : 1125000 
ymax       : 1126000 

Also the pitfree algorithm apparently has a crop back to the original extent implemented, as grid_canopy(pitfree(subcircle > 0)) also works fine:

>   r_chm = grid_canopy(tile_las, res=ref_raster, pitfree(thresholds = c(0, 2, 5, 10, 15), max_edge = c(0, 1), subcircle = 0.5) )
>   extent(r_chm_bottom)
class      : Extent 
xmin       : 2693000 
xmax       : 2694000 
ymin       : 1125000 
ymax       : 1126000 

I suggest having a p2r optional parameter cropToInputExtent = TRUE (you may have a more concise name for it ;] ), that allows for extension by the subcircle tweak, but defaults to extent(input) = extent(output) [as generally may be expected and allows for usage of a reference raster grid_canopy(las, res=raster(extent(las), res=1), p2r(0.5)) ].

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Dec 9, 2021

C++ unexpected internal error in 'rasterize': point out of raster.

This problem has been fixed in #483 and released in 3.2.2. Which version are you using?

t can be argued that it makes sense, that the user gets as chm the full extent of the "extended" point cloud - although having a larger output extent than input extent may still be surprising to some.

I do agree and this is something I was thinking about for a while. I removed this in v4. The extent of the point cloud is now fixed and is not affected by the subcircle tweak

grid_metrics(~max(Z)) basically equivalent to grid_canopy(p2r())

Yes, but ten times slower approximately.

I suggest having a p2r optional parameter cropToInputExtent = TRUE

I think that do not extending the raster is simpler, more consistent and more logic.

@Jean-Romain Jean-Romain self-assigned this Dec 9, 2021
@Jean-Romain Jean-Romain added the Enhancement Not actually a bug but a possible improvement label Dec 9, 2021
Jean-Romain added a commit that referenced this issue Dec 9, 2021
@Hannes-Ole
Copy link
Author

You are right, I was working on a server where still lidR 3.2.1 was installed, so this is obsolete then.
I also agree that not extending the raster is consistent and the optional extension isn't really necessary.
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Not actually a bug but a possible improvement
Projects
None yet
Development

No branches or pull requests

2 participants