vulnerability to PROJ6/GDAL3 issues

[rsbivand]

sp and rgdal are changing how they handle CRS. See i.e. https://www.r-spatial.org/r/2020/03/17/wkt.html. I see:

* checking tests ... ERROR
  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Complete output:
  > library(testthat)
  > library(spatsoc)
  > 
  > test_check("spatsoc")
  ── 1. Failure: hrParams can have both vertices and kernel args (@test-build-poly
  `build_polys(...)` produced warnings.
  
  ══ testthat results  ═══════════════════════════════════════════════════════════
  [ OK: 267 | SKIPPED: 0 | WARNINGS: 82 | FAILED: 1 ]
  1. Failure: hrParams can have both vertices and kernel args (@test-build-polys.R#253) 
  
  Error: testthat unit tests failed
  Execution halted

when testing with released sp and devel rgdal on PROJ7/GDAL3.

[robitalec]

Thank you for your notice @rsbivand.

I've installed PROJ7/GDAL3 locally and ensured my sp version is up to date.

How can I install the devel rgdal?
Using install.packages("rgdal", repos="http://R-Forge.R-project.org")..

[rsbivand]

If you prefer to use Windows, see https://stat.ethz.ch/pipermail/r-sig-geo/2020-April/028063.html for current sp and rgdal binaries. Otherwise on Linux and source installs, use install.packages("rgdal", repos="http://R-Forge.R-project.org") as you found, but also use remotes::install_github("rsbivand/sp") to get sp 1.4-2.

[robitalec]

Thanks @rsbivand.

Install of rgdal using install.packages("rgdal", repos="http://R-Forge.R-project.org") is giving me an error.
I had to build proj 7 from the released .tar.gz because it hasn't been updated for Arch (or Fedora, Debian as far as I can tell).

I don't see a "BugReports" in the Description for rgdal so we could discuss it here, or let me know where you'd prefer me to post it.

* installing *source* package ‘rgdal’ ...
** using staged installation
configure: R_HOME: /usr/lib64/R
configure: CC: gcc
configure: CXX: g++ -std=gnu++11
configure: C++11 support available
configure: rgdal: 1.5-7
checking for /usr/bin/svnversion... yes
cat: inst/SVN_VERSION: No such file or directory
configure: svn revision: 
checking for gdal-config... /usr/bin/gdal-config
checking gdal-config usability... yes
configure: GDAL: 3.0.4
checking GDAL version >= 1.11.4... yes
checking GDAL version <= 2.5 or >= 3.0... yes
checking gdal: linking with --libs only... yes
checking GDAL: gdal-config data directory readable... yes
checking GDAL: /usr/share/gdal/stateplane.csv readable... yes
configure: pkg-config proj exists, will use it
configure: PROJ version: 7.0.0
configure: proj CPP flags: -DPROJ_H_API -I/usr/local/include  
configure: PROJ LIBS: -L/usr/local/lib -lproj  
checking PROJ header API:... proj.h

...

** installing vignettes
** testing if installed package can be loaded from temporary location
Error: package or namespace load failed for ‘rgdal’ in dyn.load(file, DLLpath = DLLpath, ...):
 unable to load shared object '/home/alecr/R/x86_64-pc-linux-gnu-library/3.5/00LOCK-rgdal/00new/rgdal/libs/rgdal.so':
  /home/alecr/R/x86_64-pc-linux-gnu-library/3.5/00LOCK-rgdal/00new/rgdal/libs/rgdal.so: undefined symbol: proj_context_is_network_enabled

[rsbivand]

This looks like the standard case of more than one libproj.so present on your system. The package is built using the library pointed at by pkg-config proj, but this is not the one found on load. Maybe you only have one libproj.so but you didn't run ldconfig, or maybe another libproj.so is earlier in the load path than the one used for building. Remove the other ones to disambiguate. There was no need to update PROJ, the same error would show with 6.3, I think. Also remember to re-install GDAL using the PROJ you just updated.

[robitalec]

Thanks you are right. I didn't see the warnings originally with PROJ 6.3 and wasn't sure since above you mention different versions in the title (PROJ6) and your original comment (PROJ7).

I fixed the install and I'm seeing the warnings with GDAL3, PROJ6.3 and released sp, devel rgdal. Thanks @rsbivand, I will take and a look to address these warnings.

[robitalec]

@rsbivand, it turns out these warnings are coming from one of spatsoc's dependencies, adehabitatHR.

I'm working on updating the CRS usage to eg. CRS("+init=epsg:4326") throughout the man and vignettes. Should I open an issue in adehabitat or have they already been alerted of upcoming PROJ/GDAL changes?

[rsbivand]

@robitalec You need a reproducible example, you have not said that you have tried to resolve this yourself. You are most likely making unreasonable assumptions about another package. I see:

Complete output:
  > library(testthat)
  > library(spatsoc)
  > 
  > test_check("spatsoc")
  ── 1. Failure: hrParams can have both vertices and kernel args (@test-build-poly
  `build_polys(...)` produced warnings.
  
  ══ testthat results  ═══════════════════════════════════════════════════════════
  [ OK: 267 | SKIPPED: 0 | WARNINGS: 82 | FAILED: 1 ]
  1. Failure: hrParams can have both vertices and kernel args (@test-build-polys.R#253) 
  
  Error: testthat unit tests failed

> library(spatsoc)
> 
> test_check("spatsoc")
── 1. Failure: hrParams can have both vertices and kernel args (@test-build-poly
`build_polys(...)` produced warnings.

══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 267 | SKIPPED: 0 | WARNINGS: 82 | FAILED: 1 ]
1. Failure: hrParams can have both vertices and kernel args (@test-build-polys.R#253) 

test_that('hrParams can have both vertices and kernel args', {
  expect_silent(
    build_polys(
      DT = DT,
      projection = utm,
      hrType = 'kernel',
      hrParams = list(percent = 95, grid = 60),
      coords = c('X', 'Y'),
      id = 'ID'
    )
  )

})

Running just those lines of your testthat file shows that you are expecting silence but getting warnings from rgdal:

> build_polys(
+       DT = DT,
+       projection = utm,
+       hrType = 'kernel',
+       hrParams = list(percent = 95, grid = 60),
+       coords = c('X', 'Y'),
+       id = 'ID'
+     )
Object of class "SpatialPolygonsDataFrame" (package sp):

Number of SpatialPolygons:  10

Variables measured:
  id      area
A  A 4264.1435
B  B  742.6162
C  C 6541.1059
D  D  371.4554
E  E 1538.9156
F  F 1551.4316
...

There were 31 warnings (use warnings() to see them)
> warnings()
Warning messages:
1: In proj4string(xy) : CRS object has comment, which is lost in output
2: In proj4string(xy) : CRS object has comment, which is lost in output
3: In proj4string(xy) : CRS object has comment, which is lost in output
4: In proj4string(xy) : CRS object has comment, which is lost in output
5: In proj4string(xy) : CRS object has comment, which is lost in output
6: In proj4string(xy) : CRS object has comment, which is lost in output
7: In proj4string(xy) : CRS object has comment, which is lost in output
8: In proj4string(xy) : CRS object has comment, which is lost in output
9: In proj4string(xy) : CRS object has comment, which is lost in output
10: In proj4string(xy) : CRS object has comment, which is lost in output
11: In proj4string(xy) : CRS object has comment, which is lost in output
12: In proj4string(x) : CRS object has comment, which is lost in output
13: In proj4string(x) : CRS object has comment, which is lost in output
14: In proj4string(x) : CRS object has comment, which is lost in output
15: In proj4string(x) : CRS object has comment, which is lost in output
16: In proj4string(x) : CRS object has comment, which is lost in output
17: In proj4string(x) : CRS object has comment, which is lost in output

Do read https://rgdal.r-forge.r-project.org/articles/CRS_projections_transformations.html and its links, and realise that expecting silence in testthat is imposing your wishes on the diligent providers of packages you use, who are warning you that your workflow is vulnerable to changes in the representation of coordinate reference systems. The problem is your use of testthat, not the correct behaviour of upstream packages. It would be much worse if we let you assume that your output should be the same but is not, by not warning you of significant upstream changes. The warninga are coming from sp and rgdal.

[rsbivand]

If you read the start-up messages given by rgdal, you will see a mechanism that may be used. It does not yet remove warnings generated by sp; I expect to use the same option settinng for sp. However, I advise most strongly against suppressing informative warnings. Say someone is using your package in production, and is in scripts messing about with CRS objects. If you have suppressed the warnings, they will not be alerted to potentially major positional degradation in their workflow. From sp 1.5 and rgdal 1.6, I'll reverse the warning defaults, but not before. If testthat is playing up (expect_silent()), either change to tinytest which can report (I understand) changed output without failing like classical diff tests, or correct your test.

[rsbivand]

So please suggest patches for rgdal and PRs for sp. As you maybe also know, EPSG:4326 by definition expects coordinates to be Northing:Easting, not regular GIS order, so it certainly is not a guarantee of clarity. The message we must get out is that all users should verify their workflows irrespective of false positive warnings. Think of the extra cost of not only checking whether a Datum present in the internal representation is dropped by GDAL's exportToProj4(), but also checking that it was WGS84. Maybe at your relevant scales, 125m doesn't matter, in which case define only the ellipsoid? I don't know all of the accuracy/scale constraints of users of your package. Sonce I do not know, I prefer to alert, to shift the responsibility from me to the end user who is more likely to know.

[robitalec]

@MartinBeal - might be best to move the discussion of your package's specific CRS/PROJ/GDAL/etc challenges to an issue there. Hiding those comments from this thread.

@rsbivand - I did investigate as mentioned here but you're right I should've posted the reproducible example. I had other pressing deadlines come up the past bit, but will have a PR addressing the test and silencing issues. Thank you.

[rsbivand]

[@robitalec]: this comment has been removed because it does not respect spatsoc's code of conduct. It is unprofessional and disrespectful to criticize other's priorities and especially negligent of other's life circumstances, which are always different than our own.

[robitalec]

For other's reference in case, I separated warnings, messages and results in tests using testthat::evaluate_promise. See commits in #31 for details. Vignettes, man, etc updated in addition to tests. Heading to CRAN now.

[rsbivand]

I further kindly suggest that you review your use of your code of conduct. Maybe chasing many unresponsive maintainers makes me impatient, but running ~900 reverse dependency checks several times weekly and handling their results by posting issues and emailing does, I feel, justify abruptness for example when a candidate resolution has been proposed and when a package has been archived unnecessarily.

[robitalec]

No Roger, we remain resolute in the use of our code of conduct. There is no place, let alone excuse, for your disrespectful remarks or behaviour like them. We expect a higher standard of respect between the authors, users and contributors of this package than you have shown in this thread. If you have any other code-related comments, please open a new issue.