Please remove dependencies on **rgdal**, **rgeos**, and/or **maptools** #178
Comments
|
maptools was only used by inactive code and has now been removed. I've also removed what looked like the last bit of remaining direct rgdal use, a showSRID call, and a call to The remaining rgdal use is all about setting the warning levels about the PROJ4/6 transition; I assume it should be ok to drop PROJ4 backwards compatibility now, which will then allow removal of a lot of code as well as allow removal of the rgdal dependency. Other remaining uses of e.g. sp and raster are help functions for plotting etc; Those will need to remain in the package as long as feasible to retain backwards compatibility as much as possible with old analysis code etc. (but I'll add some warning system). Most of the sf/sp related code will be transferred to a separate |
|
Thanks! Yes, I think that the PROJ.4 fallback may be dropped now - revdep checks should show any remaining cases. I still find old |
|
The remaining rgeos and rgdal uses in INLA have now been removed, so that will be sorted with the next INLA build (next week). There is a major bug in I also believe that If my understanding is correct, I can make a pull request for a correction to |
|
Please update sp from github, and see whether |
|
That gets rid of the warnings, and highlights the |
|
Yes, could you provide a simple PR, please? |
|
With sp 1.6-0 installed and rgdal not installed, with the default evolution status 0L (and also with 1L) things are failing in subtle ways; I traced one issue (which may be the only/main issue) to The main issue at the moment is that inlabru calls some of the affected INLA methods, so tests for inlabru fail under evolution status 0L when removing the rgdal dependency, so I'm not sure how to handle this in the next CRAN submission of inlabru. I could perhaps add a check in inlabru |
|
Thanks! sp has always, since rgdal CRS support reached CRAN, checked for rgdal and used it for CRS checking if present. I'll see whether anything got changed, but without rgdal, sp should simply use the provided PROJ4 string, IIRC, in status 0L and 1L. Is it possible that the sp CRS cache (use_cache=TRUE by default) is causing problems? It was added as I saw some packages using CRS() repeatedly with the same input argument, sometimes thousands of times, and time taken in PROJ code to convert to WKT2 was noticable. |
|
The issue the inla methods are running into is when specifying only a SRS_string as input, with contents that wouldn’t necessarily be translatable into a proj4 string (or rather, that sp/rgdal in the past wouldn’t necessarily know how to convert to proj4, like some combinations involving units=km). |
|
Please could you provide a reprex? |
|
With your PR edzer/sp#128 applied locally: so without a reprex it is hard to tell. With current inlabru/devel: |
|
I wasn't entirely clear, sorry. reprex below. It's not technically a bug per se, as the which implies it's ignored when rgdal isn't installed, thus breaking code that we wrote to work with WKT definitions as the prime information carrier instead of PROJ4, back when the PROJ4/6 transition took place. But as it does use it when evolution status is library(sp)
set_evolution_status(0L)
#> [1] 0
c1_CRS <- CRS("+proj=longlat +R=1 +no_defs")
c1_crs <- sf::st_crs(c1_CRS)
c1_crs$wkt
#> [1] "GEOGCRS[\"unknown\",\n DATUM[\"unknown\",\n ELLIPSOID[\"unknown\",1,0,\n LENGTHUNIT[\"metre\",1,\n ID[\"EPSG\",9001]]]],\n PRIMEM[\"Reference meridian\",0,\n ANGLEUNIT[\"degree\",0.0174532925199433,\n ID[\"EPSG\",9122]]],\n CS[ellipsoidal,2],\n AXIS[\"longitude\",east,\n ORDER[1],\n ANGLEUNIT[\"degree\",0.0174532925199433,\n ID[\"EPSG\",9122]]],\n AXIS[\"latitude\",north,\n ORDER[2],\n ANGLEUNIT[\"degree\",0.0174532925199433,\n ID[\"EPSG\",9122]]]]"
c2_CRS <- CRS(SRS_string = c1_crs$wkt)
# NA since SRS_string only used when rgdal available :
c2_CRS
#> Coordinate Reference System:
#> Deprecated Proj.4 representation: NA
set_evolution_status(1L)
#> [1] 1
c1_CRS <- CRS("+proj=longlat +R=1 +no_defs")
c1_crs <- sf::st_crs(c1_CRS)
c1_crs$wkt
#> [1] "GEOGCRS[\"unknown\",\n DATUM[\"unknown\",\n ELLIPSOID[\"unknown\",1,0,\n LENGTHUNIT[\"metre\",1,\n ID[\"EPSG\",9001]]]],\n PRIMEM[\"Reference meridian\",0,\n ANGLEUNIT[\"degree\",0.0174532925199433,\n ID[\"EPSG\",9122]]],\n CS[ellipsoidal,2],\n AXIS[\"longitude\",east,\n ORDER[1],\n ANGLEUNIT[\"degree\",0.0174532925199433,\n ID[\"EPSG\",9122]]],\n AXIS[\"latitude\",north,\n ORDER[2],\n ANGLEUNIT[\"degree\",0.0174532925199433,\n ID[\"EPSG\",9122]]]]"
c2_CRS <- CRS(SRS_string = c1_crs$wkt)
# NA since SRS_string only used when rgdal available:
c2_CRS
#> Coordinate Reference System:
#> Deprecated Proj.4 representation: NA
set_evolution_status(2L)
#> [1] 2
c1_CRS <- CRS("+proj=longlat +R=1 +no_defs")
c1_crs <- sf::st_crs(c1_CRS)
c1_crs$wkt
#> [1] "GEOGCRS[\"unknown\",\n DATUM[\"unknown\",\n ELLIPSOID[\"unknown\",1,0,\n LENGTHUNIT[\"metre\",1,\n ID[\"EPSG\",9001]]]],\n PRIMEM[\"Reference meridian\",0,\n ANGLEUNIT[\"degree\",0.0174532925199433,\n ID[\"EPSG\",9122]]],\n CS[ellipsoidal,2],\n AXIS[\"longitude\",east,\n ORDER[1],\n ANGLEUNIT[\"degree\",0.0174532925199433,\n ID[\"EPSG\",9122]]],\n AXIS[\"latitude\",north,\n ORDER[2],\n ANGLEUNIT[\"degree\",0.0174532925199433,\n ID[\"EPSG\",9122]]]]"
c2_CRS <- CRS(SRS_string = c1_crs$wkt)
# Other method used, both wkt and projargs (derived from the wkt) set correctly:
c2_CRS
#> Coordinate Reference System:
#> Deprecated Proj.4 representation: +proj=longlat +R=1 +no_defs
#> WKT2 2019 representation:
#> GEOGCRS["unknown",
#> DATUM["unknown",
#> ELLIPSOID["unknown",1,0,
#> LENGTHUNIT["metre",1,
#> ID["EPSG",9001]]]],
#> PRIMEM["Reference meridian",0,
#> ANGLEUNIT["degree",0.0174532925199433,
#> ID["EPSG",9122]]],
#> CS[ellipsoidal,2],
#> AXIS["longitude",east,
#> ORDER[1],
#> ANGLEUNIT["degree",0.0174532925199433,
#> ID["EPSG",9122]]],
#> AXIS["latitude",north,
#> ORDER[2],
#> ANGLEUNIT["degree",0.0174532925199433,
#> ID["EPSG",9122]]]] |
|
Looking more closely, I now realise that the WKT information isn't set in the initial CRS constructions at all when given projargs inputs, even when evolution status is 2L for these tests, which is another issue. library(sp)
set_evolution_status(0L)
#> [1] 0
c1_CRS <- CRS("+proj=longlat")
str(c1_CRS)
#> Formal class 'CRS' [package "sp"] with 1 slot
#> ..@ projargs: chr "+proj=longlat"
set_evolution_status(1L)
#> [1] 1
c1_CRS <- CRS("+proj=longlat")
str(c1_CRS)
#> Formal class 'CRS' [package "sp"] with 1 slot
#> ..@ projargs: chr "+proj=longlat"
set_evolution_status(2L)
#> [1] 2
c1_CRS <- CRS("+proj=longlat")
str(c1_CRS)
#> Formal class 'CRS' [package "sp"] with 1 slot
#> ..@ projargs: chr "+proj=longlat" |
|
I see that the may be a bit overzealous now; it seems other packages are now more relaxed about accepting proj4 strings as inputs, and there's no hindrance for us doing that as well, as long as we can rely of |
|
You cannot rely on any proj4 string in general. You would have first to be sure that it does not contain and There are no projection or transformation pipelines for any of these, and getting adequate pipelines is what the changes over the last 6 years have been about (in addition to adding epochs for plate tectonics). I'm unsure what you need. Do you want |
|
It's precisely to avoid relying on proj4 inputs that we've been using the SRS_string argument instead, and we explicitly do not care about whether the proj4 string is populated by something readable or not. So under evolution status 0 and 1, with no rgdal installed, everything breaks down as it ignores the valid wkt specification input. I can tweak the internal inlabru logic a bit to workaround the issue, with an approach that's similar to evolution status 2L, by going via library(sp)
library(inlabru)
set_evolution_status(0L)
#> [1] 0
c1_CRS <- fm_CRS(fm_crs(CRS("+proj=longlat")))
str(c1_CRS)
#> Formal class 'CRS' [package "sp"] with 1 slot
#> ..@ projargs: chr "+proj=longlat +datum=WGS84 +no_defs"
#> ..$ comment: chr "GEOGCRS[\"unknown\",\n DATUM[\"World Geodetic System 1984\",\n ELLIPSOID[\"WGS 84\",6378137,298.25722"| __truncated__
set_evolution_status(1L)
#> [1] 1
c1_CRS <- fm_CRS(fm_crs(CRS("+proj=longlat")))
str(c1_CRS)
#> Formal class 'CRS' [package "sp"] with 1 slot
#> ..@ projargs: chr "+proj=longlat +datum=WGS84 +no_defs"
#> ..$ comment: chr "GEOGCRS[\"unknown\",\n DATUM[\"World Geodetic System 1984\",\n ELLIPSOID[\"WGS 84\",6378137,298.25722"| __truncated__
set_evolution_status(2L)
#> [1] 2
c1_CRS <- fm_CRS(fm_crs(CRS("+proj=longlat")))
str(c1_CRS)
#> Formal class 'CRS' [package "sp"] with 1 slot
#> ..@ projargs: chr "+proj=longlat +datum=WGS84 +no_defs"
#> ..$ comment: chr "GEOGCRS[\"unknown\",\n DATUM[\"World Geodetic System 1984\",\n ELLIPSOID[\"WGS 84\",6378137,298.25722"| __truncated__One of the core issues we have to deal with in inlabru/INLA are geocentric coordinate system, as well as (for numerical stability reasons) scaled versions of the globe in such systems, in particular the radius 1 sphere. The As for |
|
To clarify: the CRS constructions with proj4 inputs in the test cases above are mostly there to help generate the wkt info that we are having problems with. I don't want to use proj4 inputs, I want to use wkt inputs. |
|
I think I understand. Is the user setting one where And it's not so much GIS uses as geodetics uses, so a long way from global analyses. |
|
I'm not sure if that's worth spending time on for sp; I was under the impression that in the not too distant future it would default to evolution status 2L, and that the problems above would mostly be a transitional problem? Are "non-standard" WKT2 crs information likely to be an issue also when working with sf? If so, that's a bigger issue. So far things have seemed ok at least; I've had to expand the inlabru specialised wkt-parser code a bit (this will be in fmesher later, but should probably be a separate package if there was a more general need for it). |
|
In terms of time-line, you are probably correct. The next |
What do you mean by that? WKT-2 is a standard, the upstream software (PROJ, GDAL) used by sf is supposed to implement it.
Hopefully, yes. |
|
“Non-standard” only as in “manually or programmatically constructed as a valid CRS specification that doesn’t correspond to the usual predefined CRS values”. Specifically, with units not in metres, and/or different ellipsoid radius. In the past, these tended to cause issues with code that expected the wit to be convertible to proj4, but the converter failed. This seems to be less of an issue with the sf CRS interface, but the transition from sp and backwards compatibility support is the issue here. |
|
Like the missing (NA) construct that we have for writing vector objects in sf, but that construct is in the upstream unit tests. |
I don't see why a package cannot use |
|
To do that we would need to capture the current s2 usage state and reset it after with on.exit or similar, so we don't break the user's own code, in case they rely on it being activated or not activated. So yes, we could do this, but at a major maintainability cost for our code. And yes, we do do geometric operations, and can encounter use cases where long-lat need to be treated as raw coordinates, as well as use cases where spherical geometry is the relevant geometry. But this wasn't really the topic here, which is about CRS specification&storage. The sf storage works much better than current sp, which is good, but we do still need to deal with sp code for backwards compatibility of a large set of legacy code. |
|
@rsbivand , I'm trying out the following sp status checking and loading code, for detecting and warning about evo status < 2 when rgdal isn't installed. The idea is to call this at some key places in the inlabru code (and likely INLA code). # Return TRUE if no sp issue detected, and `requireNamespace("sp")`
# Return FALSE if a potential issue is detected, and give a warning if
# `silent` is `FALSE`
# Give an error if sp not available
check_sp_status <- function(silent = FALSE) {
sp_version <- tryCatch(packageVersion("sp"),
error = function(e) NA_character_)
if (is.na(sp_version)) {
if (!silent) {
stop("No 'sp' version detected.")
}
return(FALSE)
}
if (sp_version >= "1.6-0") {
stopifnot(requireNamespace("sp"))
# Default to 2L to allow future sp to stop supporting
# get_evolution_status; assume everything is fine if it fails.
evolution_status <- tryCatch(sp::get_evolution_status(),
error = function(e) 2L)
rgdal_version <- tryCatch(packageVersion("rgdal"),
error = function(e) NA_character_)
if ((evolution_status < 2L) && is.na(rgdal_version)) {
if (!silent) {
warning(
paste0(
"'sp' version >= 1.6-0 detected, rgdal isn't installed, and evolution status is < 2L.\n",
"This may cause issues with some CRS handling code. To avoid this, use 'sp::set_evolution_status(2L)'"
)
)
}
return(FALSE)
}
}
return(TRUE)
} |
|
This looks as though it covers the range of alternatives. Could this be in |
|
I made a slight inconsistency; it was meant to load sp if available, regardless of version. Part of the function wouldn’t make sense to be in sp itself I think. But if you think parts of it would make sense in sp or sf, that would be fine! |
|
Aha, now I see what you meant. It’s mostly the requireNamespace(“sp”) that wouldn’t make sense inside the sp package itself… |
|
I forgot about the automated inlabru examples checking, where the evolution status won't have been set bu the user. For the testthat tests I set it in a setup function that's called at the top of each test file. For the Since Setting the status in |
…l a proper fix is implemented. #178
… optionally force safe sp usage regarding rgdal and sf. #178
|
Running CMD check in a scenario using sp evolution status 2 (substitute use of rgdal with sf for projection/transformation/CRS) and absence of retiring packages from the library path passes as in this log: |
|
I've done an experimental commit now that removes the rgdal dependency and protects examples with |
|
User code will still be unsafe since it's not protected by default; I'll likely add some calls to |
|
I would like to try to do another project report, for users with workflows and teaching materials rather than package maintainers. To start with, suggesting that they should run diffs on output with retiring packages present, then absent, to control for any regressions. But putting in warnings seems to nudge them into suppressing the warnings rather than reading them. We could even do an INLA blog for INLA users, if that might be helpful. |
|
When the CRS handling fails in inlabru/INLA user code, the effect can be much later in the code and/or with cryptic error messages that aren't always obviously related to the root cause. For example, if a technically valid crs is created but becomes NA instead of what it was meant to be, then the effect can be that it ends up evaluating covariates at the wrong location, possibly outside the domain, creating NA or NULL as data input, and tracing those kinds of errors in user can be extremely tedious and time consuming for me, as it tends to involve those who don't know how to debug themselves, so flagging the problem should ideally be done as early as possible. I'd be ok with using We'll update the example data sets to sf/terra, but that will likely need to be under different names, since the formats and applicable methods are so different. There will definitely need to be a document for INLA users, but this will need to involve the |
|
An unfortunate side effect of working around the problem in the inlabru examples with |
|
Option for the package data: rename the existing data like |
|
We are planning on converting the package examples/vignettes user-side code from sp to sf soon. Some already convert to sf. |
|
From June 2023, |
|
OK, June 2023 is very soon, so that's good! Then I feel ok with adding a few Yes, feel free to link to here! |
|
Maybe the message could suggest frequent updates of packages as a good habit (but of course recording package versions used in generating results from workflows)? |
|
Solved by #193 |
|
|
This package depends on (depends, imports or suggests) raster and one or more of the retiring packages rgdal, rgeos or maptools (https://r-spatial.org/r/2022/04/12/evolution.html). Since raster
3.6.3, all use of external FOSS library functionality has been transferred to terra, making the retiring packages very likely redundant. It would help greatly if you could remove dependencies on the retiring packages as soon as possible.The text was updated successfully, but these errors were encountered: