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

lidR vulnerable to forthcoming changes in sp and rgdal #299

Closed
rsbivand opened this issue Nov 16, 2019 · 13 comments
Closed

lidR vulnerable to forthcoming changes in sp and rgdal #299

rsbivand opened this issue Nov 16, 2019 · 13 comments
Assignees
Labels
CRAN A change that is required by the CRAN

Comments

@rsbivand
Copy link

Running revdep checks for current rgdal on R-Forge - see:

https://stat.ethz.ch/pipermail/r-sig-geo/2019-November/027801.html

shows the errors in the attached test failure log, related to use of PROJ&/GDAL3
and required changes to sp and rgdal. If useful find a regerence to a docker
image in this thread:

r-spatial/discuss#28

Changes will occur quite fast, and packages need to be prepared.

R version 3.6.1 (2019-07-05) -- "Action of the Toes"
Copyright (C) 2019 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> Sys.setenv("R_TESTS" = "")
> 
> library(testthat)
> library(lidR)
Loading required package: raster
Loading required package: sp
> 
> options(lidR.progress = FALSE)
> 
> test_check("lidR")
── 1. Failure: LAS builds a LAS object with a CRS (@test-LAS.R#85)  ────────────
projection(las2) not equal to "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs +ellps=GRS80 +towgs84=0,0,0".
1/1 mismatches
x[1]: "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs"
y[1]: "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs +ellps=GRS80 +towgs84=0
y[1]: ,0,0"

── 2. Failure: LAS builds a LAS object with a CRS (@test-LAS.R#90)  ────────────
projection(las2) not equal to "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs +ellps=GRS80 +towgs84=0,0,0".
1/1 mismatches
x[1]: "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs"
y[1]: "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs +ellps=GRS80 +towgs84=0
y[1]: ,0,0"

── 3. Failure: grid_canopy tin works both with LAS and LAScatalog (@test-grid_ca
x@crs not equal to las@proj4string.
Attributes: < Component "comment": 1 string mismatch >

── 4. Failure: grid_canopy pit-free works both with LAS and LAScatalog (@test-gr
x@crs not equal to las@proj4string.
Attributes: < Component "comment": 1 string mismatch >

── 5. Failure: projection with wkt code works (@test-projection.R#27)  ─────────
las@header@VLR$`WKT OGC CS`$`WKT OGC COORDINATE SYSTEM` not equal to "PROJCS[\"NAD_1983_UTM_Zone_17N\",GEOGCS[\"GCS_North_American_1983\",DATUM[\"D_North_American_1983\",SPHEROID[\"GRS_1980\",6378137,298.257222101]],PRIMEM[\"Greenwich\",0],UNIT[\"Degree\",0.017453292519943295]],PROJECTION[\"Transverse_Mercator\"],PARAMETER[\"latitude_of_origin\",0],PARAMETER[\"central_meridian\",-81],PARAMETER[\"scale_factor\",0.9996],PARAMETER[\"false_easting\",500000],PARAMETER[\"false_northing\",0],UNIT[\"Meter\",1]]".
1/1 mismatches
x[1]: "PROJCS[\"unknown\",GEOGCS[\"GCS_unknown\",DATUM[\"D_North_American_1983\"
x[1]: ,SPHEROID[\"GRS_1980\",6378137.0,298.257222101]],PRIMEM[\"Greenwich\",0.0]
x[1]: ,UNIT[\"Degree\",0.0174532925199433]],PROJECTION[\"Transverse_Mercator\"],
x[1]: PARAMETER[\"False_Easting\",500000.0],PARAMETER[\"False_Northing\",0.0],PA
x[1]: RAMETER[\"Central_Meridian\",-81.0],PARAMETER[\"Scale_Factor\",0.9996],...
y[1]: "PROJCS[\"NAD_1983_UTM_Zone_17N\",GEOGCS[\"GCS_North_American_1983\",DATUM
y[1]: [\"D_North_American_1983\",SPHEROID[\"GRS_1980\",6378137,298.257222101]],P
y[1]: RIMEM[\"Greenwich\",0],UNIT[\"Degree\",0.017453292519943295]],PROJECTION[\
y[1]: "Transverse_Mercator\"],PARAMETER[\"latitude_of_origin\",0],PARAMETER[\"ce
y[1]: ntral_meridian\",-81],PARAMETER[\"scale_factor\",0.9996],PARAMETER[\"fa...

── 6. Failure: projection with wkt code works (@test-projection.R#32)  ─────────
las@header@VLR$`WKT OGC CS`$`WKT OGC COORDINATE SYSTEM` not equal to "PROJCS[\"NAD_1983_UTM_Zone_18N\",GEOGCS[\"GCS_North_American_1983\",DATUM[\"D_North_American_1983\",SPHEROID[\"GRS_1980\",6378137,298.257222101]],PRIMEM[\"Greenwich\",0],UNIT[\"Degree\",0.017453292519943295]],PROJECTION[\"Transverse_Mercator\"],PARAMETER[\"latitude_of_origin\",0],PARAMETER[\"central_meridian\",-75],PARAMETER[\"scale_factor\",0.9996],PARAMETER[\"false_easting\",500000],PARAMETER[\"false_northing\",0],UNIT[\"Meter\",1]]".
1/1 mismatches
x[1]: "PROJCS[\"unknown\",GEOGCS[\"GCS_unknown\",DATUM[\"D_North_American_1983\"
x[1]: ,SPHEROID[\"GRS_1980\",6378137.0,298.257222101]],PRIMEM[\"Greenwich\",0.0]
x[1]: ,UNIT[\"Degree\",0.0174532925199433]],PROJECTION[\"Transverse_Mercator\"],
x[1]: PARAMETER[\"False_Easting\",500000.0],PARAMETER[\"False_Northing\",0.0],PA
x[1]: RAMETER[\"Central_Meridian\",-75.0],PARAMETER[\"Scale_Factor\",0.9996],...
y[1]: "PROJCS[\"NAD_1983_UTM_Zone_18N\",GEOGCS[\"GCS_North_American_1983\",DATUM
y[1]: [\"D_North_American_1983\",SPHEROID[\"GRS_1980\",6378137,298.257222101]],P
y[1]: RIMEM[\"Greenwich\",0],UNIT[\"Degree\",0.017453292519943295]],PROJECTION[\
y[1]: "Transverse_Mercator\"],PARAMETER[\"latitude_of_origin\",0],PARAMETER[\"ce
y[1]: ntral_meridian\",-75],PARAMETER[\"scale_factor\",0.9996],PARAMETER[\"fa...

══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 595 | SKIPPED: 0 | WARNINGS: 0 | FAILED: 6 ]
1. Failure: LAS builds a LAS object with a CRS (@test-LAS.R#85) 
2. Failure: LAS builds a LAS object with a CRS (@test-LAS.R#90) 
3. Failure: grid_canopy tin works both with LAS and LAScatalog (@test-grid_canopy.R#71) 
4. Failure: grid_canopy pit-free works both with LAS and LAScatalog (@test-grid_canopy.R#88) 
5. Failure: projection with wkt code works (@test-projection.R#27) 
6. Failure: projection with wkt code works (@test-projection.R#32) 

Error: testthat unit tests failed
Execution halted
@Jean-Romain
Copy link
Collaborator

Thank you for reporting. It does not bring much trouble and it is easy to fix in my case. I just need to find how to install gdal3 and proj6.

@Jean-Romain Jean-Romain self-assigned this Nov 16, 2019
@Jean-Romain Jean-Romain added Bug A bug in the package CRAN A change that is required by the CRAN and removed Bug A bug in the package labels Dec 12, 2019
@rsbivand
Copy link
Author

rsbivand commented Apr 1, 2020

Major problems remain as of sp 1.4-1, rgdal 1.5-6, sf 0.9-0:

* checking tests ... ERROR
  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  Error in showSRID(uprojargs, format = "WKT2", multiline = "YES") : 
    Can't parse PROJ.4-style parameter string
  Error in showSRID(uprojargs, format = "PROJ", multiline = "NO") : 
    Can't parse PROJ.4-style parameter string
  Error in showSRID(uprojargs, format = "WKT2", multiline = "YES") : 
    Can't parse PROJ.4-style parameter string
  ══ testthat results  ═══════════════════════════════════════════════════════════
  [ OK: 968 | SKIPPED: 0 | WARNINGS: 546 | FAILED: 4 ]
  1. Failure: LAS builds a LAS object with a CRS (@test-LAS.R#85) 
  2. Failure: LAS builds a LAS object with a CRS (@test-LAS.R#90) 
  3. Failure: grid_canopy tin works both with LAS and LAScatalog (@test-grid_canopy.R#83) 
  4. Failure: grid_canopy pit-free works both with LAS and LAScatalog (@test-grid_canopy.R#100) 
  
  Error: testthat unit tests failed
  Execution halted

Taking test-LAS, line 85:

library(lidR)
data    <- data.frame(X = runif(10), Y = runif(10), Z = runif(10))
LASfile <- system.file("extdata", "Megaplot.laz", package = "lidR")
las     <- readLAS(LASfile)
projection(las2)
# [1] "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs"

The internals of projection() certainly need to be adapted to ongoing changes in sf and sp. For sp, doing x <- slot(x, "proj4string") would be clearer. Certainly trimming a "CRS" object's internal slot will now risk losing the WKT2_2019 contained in the comment piggy-backed onto the "CRS".

@Jean-Romain Jean-Romain reopened this Apr 1, 2020
@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Apr 1, 2020

I'm able to reproduce and I fixed those issues that were not critical.

However I also have 500+ warnings because the following now throw a warnings:

wkt <- rgdal::showWKT("+init=epsg:2008")
p4 <- rgdal::showP4(wkt)
sp::CRS(p4)
#> Warning in showSRID(uprojargs, format = "PROJ", multiline = "NO"): Discarded
#> datum Unknown based on Clarke 1866 ellipsoid in CRS definition
#> CRS arguments:
#>  +proj=tmerc +lat_0=0 +lon_0=-55.5 +k=0.9999 +x_0=304800 +y_0=0
#> +ellps=clrk66 +units=m +no_defs

wkt <- rgdal::showWKT("+init=epsg:2949")
p4 <- rgdal::showP4(wkt)
sp::CRS(p4)
#> Warning in showSRID(uprojargs, format = "PROJ", multiline = "NO"): Discarded
#> datum Unknown based on GRS80 ellipsoid in CRS definition
#> CRS arguments:
#>  +proj=tmerc +lat_0=0 +lon_0=-70.5 +k=0.9999 +x_0=304800 +y_0=0
#> +ellps=GRS80 +units=m +no_defs

I can fixed ~250 of them with doCheckArgs = FALSE but many of them also come from other packages such as rgeos or raster with e.g.

rgeos::gUnaryUnion(spdf)
raster::projection(r) <- projection(las)

I need to be able to transform an epsg code into a proj4 that does not trigger those warnings.

> library(rgdal)
Le chargement a nécessité le package : sp
rgdal: version: 1.5-6, (SVN revision (unknown))
 Geospatial Data Abstraction Library extensions to R successfully loaded
 Loaded GDAL runtime: GDAL 3.0.4, released 2020/01/28
 Path to GDAL shared files: /usr/share/gdal
 GDAL binary built with GEOS: TRUE 
 Loaded PROJ.4 runtime: Rel. 7.0.0, March 1st, 2020, [PJ_VERSION: 700]
 Path to PROJ.4 shared files: /home/jr/.local/share/proj:/usr/share/proj
 PROJ CDN enabled: FALSE 
 Linking to sp version: 1.4-1 

@rsbivand
Copy link
Author

rsbivand commented Apr 1, 2020

Thanks for moving on this. See if rgdal::set_thin_PROJ6_warnings(TRUE) helps for reducing warnings. If you can condition on rgdal version, look at rgdal::showSRID() as a grown-up version of the earlier functions. ExportToProj4() in GDAL is very vulnerable, and we should aim to abandon Proj4 strings completely soon. I found this mailing list thread yesterday, it is almost 8 years old: https://lists.osgeo.org/pipermail/gdal-dev/2012-November/034554.html

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Apr 1, 2020

I tested rgdal::showSRID() but even the example in the doc throws a warning.

rgdal::showSRID("EPSG:2949", "PROJ")
#> Warning in rgdal::showSRID("EPSG:2949", "PROJ"): Discarded datum
#> NAD83_Canadian_Spatial_Reference_System in CRS definition
#> [1] "+proj=tmerc +lat_0=0 +lon_0=-70.5 +k=0.9999 +x_0=304800 +y_0=0 +ellps=GRS80 +units=m +no_defs"

And rgdal::set_thin_PROJ6_warnings(TRUE) does not help (it throws a new warning)

rgdal::set_thin_PROJ6_warnings(TRUE)
rgdal::showSRID("EPSG:2949", "PROJ")
#> Warning in rgdal::showSRID("EPSG:2949", "PROJ"): PROJ6/GDAL3 PROJ string degradation in workflow
#>  repeated warnings suppressed
#>  Discarded datum NAD83_Canadian_Spatial_Reference_System in CRS definition
#> [1] "+proj=tmerc +lat_0=0 +lon_0=-70.5 +k=0.9999 +x_0=304800 +y_0=0 +ellps=GRS80 +units=m +no_defs"

we should aim to abandon Proj4 strings completely soon

Ok but how we do that? RasterLayer form raster are using CRS objects. Many functions from lidR are returning Raster* with a CRS copied from the las files to the raster. Should I start a work to redesign my classes to remove the inheritance on the Spatial class form sp? That will be such a massive work.

@rsbivand
Copy link
Author

rsbivand commented Apr 1, 2020

The warnings are just warnings, they are meant to alert the user that an important part of the Proj4 representation (+datum=, in some settings +towgs84=, +ellps= etc.) has not been exported to Proj4. It is there in the WKT in sf and sp. raster/terra haven't moved but will have to when users adopt PROJ >= 6, which will happen soon (when CRAN binaries for OSX and Windows adopt). When you suppress the warnings, you get one warning and the others are suppressed, but counted. In rgdal, the warnings will be turned off by default once we feel that no unwarned damage is being done to workflows. If everybody simply reads data from files, no transformation, etc., there will not be many warnings. Cooking one's own CRS is becoming a thing of the past, if accuracy in transformation is also needed, of course not just in R. It will take a lot of work (already has) to stabilize things, I'm afraid.

@rsbivand
Copy link
Author

rsbivand commented Apr 1, 2020

And moving away from sp will not help. terra uses simple CRS, but they will break soon, and need fixing, since terra uses GDAL and GDAL uses WKT2, and will make Proj4 strings defunct.

If you have a simple test-bed script for your classes/workflow, I can look at how it might be developed to use WKT2.

@Jean-Romain
Copy link
Collaborator

The warnings are just warnings,

Yes for sure. But on CRAN a non fixable warning is likely to be an archived package...

In rgdal, the warnings will be turned off by default once we feel that no unwarned damage is being done to workflows.

So, now the errors are fixed can I install back rgdal 1.4-8 for not being annoyed with all these warnings and wait for the CRAN release safely?

If I'm understanding well, sp, sf, raster, terra and I guess many other important packages for geospatial will move on WKT CRS when CRAN will fully adopt proj >=6. Right? For sf it is already the case. I just tested it and st_crs() gives a WKT.

If everybody simply reads data from files, no transformation, etc., there will not be many warnings

That is basically what I doing as we already discussed. I'm reading an epsg code or a WKT string (rarely) and I'm making a CRS compatible with the geospatial ecosystem (i.e. a proj4). Then, the CRS is propagated to the outputs usually Raster* or Spatial*DataFrame objects.

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Apr 1, 2020

If you have a simple test-bed script for your classes/workflow, I can look at how it might be developed to use WKT2.

There are 4 classes: LAS, LASheader, LAScatalog and LAScluster. Let put aside LAScluster because it is an internal class that can be redesigned without affecting regular code at user level.

LAS stores a LAS file and inherits from Spatial

setClass(
  Class = "LAS", contains = "Spatial",
  representation(data = "data.table", header = "LASheader"))

LASheader stores the header of a LASfile and is standalone.

LAScatalog stores an object dedicated to manipulate a collection of LAS files and inherits SpatialPolygonsDataFrame.

setClass(
  Class = "LAScatalog",
  contains = "SpatialPolygonsDataFrame",
  representation(
    chunk_options = "list",
    processing_options = "list",
    output_options = "list",
    input_options = "list" ))

Below some simple tests related to CRS that can be performed.

If sp changes the class CRS to handle a WKT instead of a proj4 and raster is updated then for me it gonna be easy. The only change I will have to do is to remove on step when parsing EPSG -> WKT -> PROJ. Less than 5 lines of code I guess + many test updates.

library(lidR)
library(testthat)
# Using rgdal 1.5-6

# Make a LAS
LASfile <- system.file("extdata", "Megaplot.laz", package = "lidR")
las <- readLAS(LASfile)

# Make a RasterLayer
bbox <- extent(las)
r <- raster(bbox)
r[] <- sample(1:ncell(r))

# Make a SpatialPointsDataFrame
spdf <- as.spatial(lasfilterground(las))

# Tests set 1
expect_equal(raster::projection(las), "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs")
expect_warning(raster::projection(r) <- raster::projection(las), NA)
expect_warning(raster::projection(spdf) <- raster::projection(las), NA)
expect_warning(sp::proj4string(spdf) <- sp::proj4string(las), NA)

# Test set 2 - change the espg code for a dummy one that is known to trigger a warning
epsg(las) <- 2008

expect_equal(raster::projection(las), "+proj=tmerc +lat_0=0 +lon_0=-55.5 +k=0.9999 +x_0=304800 +y_0=0 +ellps=clrk66 +units=m +no_defs")
expect_equal(las@header@VLR[["GeoKeyDirectoryTag"]][["tags"]][[2]][["value offset"]], 2008)
expect_warning(raster::projection(r) <- raster::projection(las), NA)
expect_warning(raster::projection(spdf) <- raster::projection(las), NA)
expect_warning(sp::proj4string(spdf) <- sp::proj4string(las), NA)

# Test set 3 - set a projection to a LAS object
projection(las) <- "+proj=utm +zone=17 +datum=WGS84 +units=m +no_defs"

expect_equal(epsg(las), 32617)

# Test set 4 - LAScatalog get attributed a CRS
ctg = readLAScatalog(LASfile)

expect_equal(sp::proj4string(ctg), "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs")

@rsbivand
Copy link
Author

rsbivand commented Apr 2, 2020

Warnings are not CRAN warnings. R CMD check just runs examples, which hard-fail only when they hit a stop() in R code, or error() in compiled code, or crash compiled code. Warnings are to alert humans who read output - if they don't read output, they will not be alerted.

Please (re-)read https://www.r-spatial.org/r/2020/03/17/wkt.html to see that is being done in sp. The useless legacy Proj4 string is retained in the "projargs" slot in "CRS" S4 objects. However, these objects now receive a comment() which most functions do not see, which contains the WKT2_2019 representation. I think that your construction now:

wkt <- rgdal::showWKT("+init=epsg:2008")
p4 <- rgdal::showP4(wkt)
sp::CRS(p4)

would simply be CRS("+init=epsg:2008") - which it should always have been. If you have a validated WKT string, then CRS(SRS_string=<wkt>) will work for GDAL >= 3 & PROJ >= 6. When GDAL < 3 & PROJ < 6, the old representations continue to work.

Please re-make the example without raster - it has not adapted to PROJ changes yet, and without testthat, just put the expected value in a comment. Probably all testing of expected values of CRS objects should wait until the situation has stabilised, say by mid-2022 (sorry, but everything has been disorganised since 2017 already, so mid-2022 is a sober estimate.

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Apr 2, 2020

Thank you for the link, this clarifies a lot the question. I'll review my code to check if I need to change something. Especially I may have used bad assignment practices in the past. The current state of the code should be free of such error but I can't swear that unit tests are all good.

would simply be CRS("+init=epsg:2008") - which it should always have been.

This is what I did so far but I changed that recently because invalid epsg codes now print a message in console (proj_create: crs not found). I was not able to get rid of that by any mean. rgdal::showWKT() fails nicely without printing. The current released code is like:

crs = tryCatch(sp::CRS("+init=epsg:1234"), error = function(x) {sp::CRS()})
#> proj_create: crs not found

SRS_string does not work with rgdal 1.4-8. It works with 1.5-6. Is it the expected behavior?

sp::CRS(SRS_string='EPSG:4326')
#> Error in sp::CRS(SRS_string = "EPSG:4326") : 
#>  no arguments in initialization list

Below a simplified set of tests. I understand I should not test the proj strings. Most of the time my tests actually aims to check is (1) the CRS was propagated to the output and (2) the CRS was properly read from files. I will see if I can simplify the tests to rely less on a strict string matching.

library(lidR) # Using rgdal 1.5-6

# Make a LAS
LASfile <- system.file("extdata", "Megaplot.laz", package = "lidR")
las <- readLAS(LASfile)

sp::proj4string(las) # "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs"

epsg(las) <- 2008
sp::proj4string(las) # "+proj=tmerc +lat_0=0 +lon_0=-55.5 +k=0.9999 +x_0=304800 +y_0=0 +ellps=clrk66 +units=m +no_defs"
epsg(las) # 2008

projection(las) <- "+proj=utm +zone=17 +datum=WGS84 +units=m +no_defs"
epsg(las) # 32617

ctg = readLAScatalog(LASfile)
sp::proj4string(ctg) # "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs"

@rsbivand
Copy link
Author

rsbivand commented Apr 2, 2020

Thanks, useful. I've edited and simplified error throwing and reporting in rgdal, in SVN revision 951 (R-forge) I see:

> library(sp)
> crs <- sp::CRS("+init=epsg:1234")
Error in showSRID(uprojargs, format = "PROJ", multiline = "NO") : 
  Can't parse PROJ.4-style parameter string
+init=epsg:1234
> crs <- try(sp::CRS("+init=epsg:1234"))
Error in showSRID(uprojargs, format = "PROJ", multiline = "NO") : 
  Can't parse PROJ.4-style parameter string
+init=epsg:1234
> if (inherits(crs, "try-error")) crs <- sp::CRS()
> crs
CRS arguments: NA 

So with R-Forge rgdal >= rev 951, you might do this:

> crs <- try(sp::CRS("+init=epsg:1234"), silent=TRUE)
> if (inherits(crs, "try-error")) crs <- sp::CRS()
> crs
CRS arguments: NA 

or maybe:

> crs <- try(sp::CRS("+init=epsg:1234"), silent=TRUE)
> if (inherits(crs, "try-error")) {
+ warning(unclass(attr(crs, "condition"))$message)
+ crs <- sp::CRS()
+ }
Warning message:
Can't parse PROJ.4-style parameter string
+init=epsg:1234 

to alert a user to the invalid string, here the EPSG code, but if you were passing a WKT, an invalid set of specifications (then via the new SRS_string= argument to CRS()).

You will only see changes in behaviour with sp 1.4-1 (CRAN) and rgdal from R-forge: install.packages("rgdal", repos="http://R-Forge.R-project.org"), installed from source and build on PROJ >= 6 and GDAL >= 3. I'm usure which you have.

I see:

> LASfile <- system.file("extdata", "Megaplot.laz", package = "lidR")
> las <- readLAS(LASfile)
> sp::proj4string(las)
[1] "+proj=utm +zone=17 +datum=NAD83 +units=m +no_defs"
Warning message:
In sp::proj4string(las) : CRS object has comment, which is lost in output
> cat(comment(slot(las, "proj4string")), "\n")
PROJCRS["NAD83 / UTM zone 17N",
    BASEGEOGCRS["NAD83",
        DATUM["North American Datum 1983",
            ELLIPSOID["GRS 1980",6378137,298.257222101,
                LENGTHUNIT["metre",1]]],
        PRIMEM["Greenwich",0,
            ANGLEUNIT["degree",0.0174532925199433]],
        ID["EPSG",4269]],
    CONVERSION["UTM zone 17N",
        METHOD["Transverse Mercator",
            ID["EPSG",9807]],
        PARAMETER["Latitude of natural origin",0,
            ANGLEUNIT["degree",0.0174532925199433],
            ID["EPSG",8801]],
        PARAMETER["Longitude of natural origin",-81,
            ANGLEUNIT["degree",0.0174532925199433],
            ID["EPSG",8802]],
        PARAMETER["Scale factor at natural origin",0.9996,
            SCALEUNIT["unity",1],
            ID["EPSG",8805]],
        PARAMETER["False easting",500000,
            LENGTHUNIT["metre",1],
            ID["EPSG",8806]],
        PARAMETER["False northing",0,
            LENGTHUNIT["metre",1],
            ID["EPSG",8807]],
        ID["EPSG",16017]],
    CS[Cartesian,2],
        AXIS["(E)",east,
            ORDER[1],
            LENGTHUNIT["metre",1,
                ID["EPSG",9001]]],
        AXIS["(N)",north,
            ORDER[2],
            LENGTHUNIT["metre",1,
                ID["EPSG",9001]]],
    USAGE[
        SCOPE["unknown"],
        AREA["North America - 84°W to 78°W and NAD83 by country"],
        BBOX[23.81,-84,84,-78]]] 

so the "CRS" object has WKT2 in its comment() as instantiated. The next step is worse:

> epsg(las1) <- 2008
Warning messages:
1: In showSRID(uprojargs, format = "PROJ", multiline = "NO") :
  Discarded datum North_American_Datum_1927_CGQ77 in CRS definition
2: In showSRID(uprojargs, format = "PROJ", multiline = "NO") :
  Discarded datum Unknown based on Clarke 1866 ellipsoid in CRS definition
> sp::proj4string(las1)
[1] "+proj=tmerc +lat_0=0 +lon_0=-55.5 +k=0.9999 +x_0=304800 +y_0=0 +ellps=clrk66 +units=m +no_defs"
Warning message:
In sp::proj4string(las1) : CRS object has comment, which is lost in output
> cat(comment(slot(las1, "proj4string")), "\n")
PROJCRS["unknown",
    BASEGEOGCRS["unknown",
        DATUM["Unknown based on Clarke 1866 ellipsoid",
            ELLIPSOID["Clarke 1866",6378206.4,294.978698213898,
                LENGTHUNIT["metre",1,
                    ID["EPSG",9001]]]],
        PRIMEM["Greenwich",0,
            ANGLEUNIT["degree",0.0174532925199433],
            ID["EPSG",8901]]],
    CONVERSION["unknown",
        METHOD["Transverse Mercator",
            ID["EPSG",9807]],
        PARAMETER["Latitude of natural origin",0,
            ANGLEUNIT["degree",0.0174532925199433],
            ID["EPSG",8801]],
        PARAMETER["Longitude of natural origin",-55.5,
            ANGLEUNIT["degree",0.0174532925199433],
            ID["EPSG",8802]],
        PARAMETER["Scale factor at natural origin",0.9999,
            SCALEUNIT["unity",1],
            ID["EPSG",8805]],
        PARAMETER["False easting",304800,
            LENGTHUNIT["metre",1],
            ID["EPSG",8806]],
        PARAMETER["False northing",0,
            LENGTHUNIT["metre",1],
            ID["EPSG",8807]]],
    CS[Cartesian,2],
        AXIS["(E)",east,
            ORDER[1],
            LENGTHUNIT["metre",1,
                ID["EPSG",9001]]],
        AXIS["(N)",north,
            ORDER[2],
            LENGTHUNIT["metre",1,
                ID["EPSG",9001]]]] 

so the datum is dropped in GDAL from the EPSG definition.

> wkt(las1)
[1] ""
> wkt(las) <- comment(slot(las, "proj4string"))
> cat(wkt(las), "\n")
PROJCRS["NAD83 / UTM zone 17N",
    BASEGEOGCRS["NAD83",
        DATUM["North American Datum 1983",
            ELLIPSOID["GRS 1980",6378137,298.257222101,
                LENGTHUNIT["metre",1]]],
        PRIMEM["Greenwich",0,
            ANGLEUNIT["degree",0.0174532925199433]],
        ID["EPSG",4269]],
    CONVERSION["UTM zone 17N",
        METHOD["Transverse Mercator",
            ID["EPSG",9807]],
        PARAMETER["Latitude of natural origin",0,
            ANGLEUNIT["degree",0.0174532925199433],
            ID["EPSG",8801]],
        PARAMETER["Longitude of natural origin",-81,
            ANGLEUNIT["degree",0.0174532925199433],
            ID["EPSG",8802]],
        PARAMETER["Scale factor at natural origin",0.9996,
            SCALEUNIT["unity",1],
            ID["EPSG",8805]],
        PARAMETER["False easting",500000,
            LENGTHUNIT["metre",1],
            ID["EPSG",8806]],
        PARAMETER["False northing",0,
            LENGTHUNIT["metre",1],
            ID["EPSG",8807]],
        ID["EPSG",16017]],
    CS[Cartesian,2],
        AXIS["(E)",east,
            ORDER[1],
            LENGTHUNIT["metre",1,
                ID["EPSG",9001]]],
        AXIS["(N)",north,
            ORDER[2],
            LENGTHUNIT["metre",1,
                ID["EPSG",9001]]],
    USAGE[
        SCOPE["unknown"],
        AREA["North America - 84°W to 78°W and NAD83 by country"],
        BBOX[23.81,-84,84,-78]]] 

I think the logic in your epsg(), epsg<-() and similar wkt() need revisiting, but it isn't too hard, I think, to trust the WKT more than the EPSG or Proj strings.

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Apr 2, 2020

When reading a las file the only available information is the epsg. So I trust the epsg. No choice. For newer formats we have WKT and in this case epsg codes are not involved. But both espg or WKT are transformed to proj to go to into a CRS and fits with geospatial ecosystem.

You will only see changes in behaviour with sp 1.4-1 (CRAN) and rgdal from R-forge:

I'm up-to-date with all geosptial stuff. I have sp 1.4.1, rgdal 1.5-6 from rforge, GDAL 3, PROJ 7 but yet I can't reproduce your warnings CRS object has comment, which is lost in output.

rgdal: version: 1.5-6, (SVN revision (unknown))
 Geospatial Data Abstraction Library extensions to R successfully loaded
 Loaded GDAL runtime: GDAL 3.0.4, released 2020/01/28
 Path to GDAL shared files: /usr/share/gdal
 GDAL binary built with GEOS: TRUE 
 Loaded PROJ.4 runtime: Rel. 7.0.0, March 1st, 2020, [PJ_VERSION: 700]
 Path to PROJ.4 shared files: /home/jr/.local/share/proj:/usr/share/proj
 PROJ CDN enabled: FALSE 
 Linking to sp version: 1.4-1 

I think the logic in your epsg(), epsg<-() and similar wkt() need revisiting, but it isn't too hard, I think, to trust the WKT more than the EPSG or Proj strings.

Well epsg() returns the epsg code read in the header. It does nothing. epsg<-() is simple (simplified version without error handling)

setMethod("epsg<-", "LAS", function(object, value)
{
  crs <- sp::CRS(paste0("+init=epsg:", value))
  epsg(object@header) <- value  # set the epsg code in the header so we can write a georeferenced files
  raster::projection(object)  <- crs # set a CRS compatible with R ecosytem.
  return(object)
})

wkt<-() is pretty much the same idea (simplified too).

setMethod("wkt<-", "LAS", function(object, value)
{
  proj <- rgdal::showP4(value) # can be replaced by sp::CRS(SRS_string = value)
  crs <- sp::CRS(proj)
  wkt(object@header) <- value # set the WKT in the header so we can write a georeferenced file
  raster::projection(object)  <- crs # set a CRS compatible with R ecosytem.
  return(object)
})

By the way you should not have been able to do wkt(las) <- comment(slot(las, "proj4string")) because the object manipulated is not in a format that support WKT. It should throw an error. I'll improve that.

Actually the more complex function is projection<-(). It takes a CRS and must retrieve the epsg code of this CRS to put that in the header of the LAS. Let say we read a file without CRS recorded. We also have a spatial object with the good CRS we can do

projection(las) <- projection(object)

And projection is more complex (but yet easy to understand). Here a sightly simplified version. It looks at the proj, converts it to WKT if the format supports WKT (LAS 1.4) or it retrieves the EPSG if the file format supports only epsg. There is room for improvement with the new comment(slot(las, "proj4string")) and I can indeed rely more on the WKT.

# x is a LAS
# value is a CRS
setMethod("projection<-", "LAS", function(x, value)
{
  proj4 <- value@projargs

  # the object is in a format that supports WKT (LAS 1.4)
  if (use_wktcs(x))
  {
    wkt <- rgdal::showWKT(proj4) # can now be replaced by comment(value)
    wkt(x@header) <- wkt
    x@proj4string <- sp::CRS(proj4)
    return(x)
  }
  # the object is in a format that supports only EPSG code
  # (might be simplified by using the WKT in `comment(value)`)
  else
  {
    # Extract epsg code (if any) using +init=epsg:xxxx part
    epsg  <- sub("\\+init=epsg:(\\d+).*",  "\\1", proj4)
    epsg  <- suppressWarnings(as.integer(epsg))

    # We were not able to extract the epsg code, we can retrieve it with rgdal
    if (is.na(epsg))  epsg <- rgdal::showEPSG(proj4)

    # We are still unable to find an epsg code
    if (epsg == "OGRERR_UNSUPPORTED_SRS")
      warning("EPSG code not found: header not updated. Try to use the function epsg() manually to ensure CRS will be written in file.", call. = FALSE)
    # We found the epsg code
    else
      epsg(x@header) <- epsg

    x@proj4string <- value
    return(x)
  }
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CRAN A change that is required by the CRAN
Projects
None yet
Development

No branches or pull requests

2 participants