r-lidar / lidR

Airborne LiDAR data manipulation and visualisation for forestry application
https://CRAN.R-project.org/package=lidR
GNU General Public License v3.0
614 stars 134 forks source link

Loading `lidR` may cause `rgrass::initGRASS` to stop with "no generic function found for 'crs'" #775

Closed Kodiologist closed 3 months ago

Kodiologist commented 3 months ago

I opened this as rsbivand/rgrass#95 originally, but the rgrass guys think the problem is with lidR. I have no idea one way or the other.

$ R --no-save -q --vanilla -e 'rgrass::initGRASS("/usr/lib/grass83", home = tempdir(), SG = terra::rast(), override = T)'
[…] # No issue
$ R --no-save -q --vanilla -e 'lidR::readLAS; rgrass::initGRASS("/usr/lib/grass83", home = tempdir(), SG = terra::rast(), override = T)'
> lidR::readLAS; rgrass::initGRASS("/usr/lib/grass83", home = tempdir(), SG = terra::rast(), override = T)
function (files, select = "*", filter = "") 
{
    if (filter == "-h" | filter == "-help") {
        rlas:::lasfilterusage()
        return(invisible())
    }
    UseMethod("readLAS", files)
}
<bytecode: 0x5a7fb97679a8>
<environment: namespace:lidR>
Error in getMethod("crs", "SpatRaster") : 
  no generic function found for 'crs'
Calls: <Anonymous> -> getMethod
Execution halted
$ R --no-save -q --vanilla -e 'lidR::readLAS; library(terra); rgrass::initGRASS("/usr/lib/grass83", home = tempdir(), SG = terra::rast(), override = T)'
[…] # No issue

I'm using:

rsbivand commented 3 months ago

The problem is that both lidR and terra define crs methods, and rgrass::initGRASS needs to discover that from terra not lidR. If terra is attached, the method is found, but if only loaded, it is hidden by the lidR method. I'll try to demonstrate this with just lidR and terra.

Jean-Romain commented 3 months ago

Well, originally lidR inherited crs() from raster. Then moving to sf/terra I created crs() for backward compatibility. At this time I'm pretty sure terra did not have crs() and was using another function. Or it was not inheritable. Or I don't know, I don't remember but I was not able to inherit from terra. The switch from sp/raster to sf/terra has not been easy.

I could inherit crs() from terra. But is is not the same signature than raster::crs() which is the one I'm using for backward compatibility.

Last solution I can remove crs(). Is is deprecated anyway and I'm now using st_crs() in lidR.

Kodiologist commented 3 months ago

Last solution I can remove crs(). Is is deprecated anyway and I'm now using st_crs() in lidR.

That seems like a reasonable choice to me.

Jean-Romain commented 3 months ago

I'm now inheriting terra::crs(), terra::is.empty() and terra::area() which were not clashing initially because they did not exist. There is no longer any name clash with terra. Please let me know it is ok for you.

Kodiologist commented 3 months ago

I think that's fine, yeah. Thanks. Are you planning a new CRAN release sometime this year?

Jean-Romain commented 3 months ago

Not really. Considering you have a workaround anyway I guess it is not urgent. If somebody needs a CRAN update I'll update.

rsbivand commented 3 months ago

If this https://github.com/r-lidar/lidR/commit/6af88b315c30d30820dc49aa152984f3640b88c6 is the relevant commit, I can try it tomorrow morning.

Jean-Romain commented 3 months ago

@rsbivand yes this is the relevant commit.

rsbivand commented 3 months ago

@Jean-Romain With your commit, this is resolved for me. thanks! @Kodiologist if you like, you may install the updated lidR source package from: https://r-lidar.r-universe.dev/lidR.