prioritizr / wdpar

Interface to the World Database on Protected Areas
https://prioritizr.github.io/wdpar
GNU General Public License v3.0
37 stars 5 forks source link

GEOS version sensitivity #50

Closed rsbivand closed 2 years ago

rsbivand commented 2 years ago

00check.log testthat.Rout.fail.log

for GEOS 3.11.0 show a failure probably because of fixes in GEOS to the NG topology engine re-ordering the coordinates of the polygon returned by sf::st_union(x)):

> st_coordinates(st_geometry(y))
        X   Y L1 L2
 [1,]  50  50  1  1
 [2,]   0  50  1  1
 [3,]  75 125  1  1
 [4,]   0 125  1  1
 [5,] 100 250  1  1
 [6,] 200 125  1  1
 [7,] 125 125  1  1
 [8,] 200  50  1  1
 [9,] 150  50  1  1
[10,] 200   0  1  1
[11,]   0   0  1  1
[12,]  50  50  1  1
> st_coordinates(st_geometry(y2))
        X   Y L1 L2
 [1,] 200   0  1  1
 [2,]   0   0  1  1
 [3,]  50  50  1  1
 [4,]   0  50  1  1
 [5,]  75 125  1  1
 [6,]   0 125  1  1
 [7,] 100 250  1  1
 [8,] 200 125  1  1
 [9,] 125 125  1  1
[10,] 200  50  1  1
[11,] 150  50  1  1
[12,] 200   0  1  1

Please adapt equality test - perhaps drop or compare areas.

jeffreyhanson commented 2 years ago

Hi @rsbivand,

Thank you very much for letting me know. I'll try and fix this as soon as I can. Just so that I'm aware of the timelines, how soon does the fix for wdpar need to be on CRAN?

rsbivand commented 2 years ago

Thanks! Timing unpredictable, GEOS 3.11.0 was just released, and CRAN waits for Debian and Fedora packages, so it depends on the upstream package maintainers' urgency or not. I have 3.11.0 installed, so please ping me when you have a fix that needs trying.

jeffreyhanson commented 2 years ago

Ok - yeah, that would be extremely helpful, I'll let you know when I've got a (potential) fix ready - thanks!

jeffreyhanson commented 2 years ago

@rsbivand, I've just pushed a fix to the fix-geos branch. When you get a chance, could you please try it out and see if that fixes it? I've updated the test to use st_equals() which returns TRUE when comparing the coordinates you posted earlier?

rsbivand commented 2 years ago

Logs for _SP_EVOLUTION_STATUS=2 _R_CHECK_FORCE_SUGGESTS_=FALSE R CMD check wdpar_1.3.2.4.tar.gz and _SP_EVOLUTION_STATUS_=2 R CMD build --no-build-vignettes wdpar-fix-geos:

00check.log

testthat.Rout.log

Looks OK! When the evolution process has moved a bit more forward, sp will be flipped from evolution status 0 using rgdal under the hood to evolution status 2 using sf under the hood, so the environment variable will cease to be needed.

jeffreyhanson commented 2 years ago

Awesome! Thank you for checking that. I'll try and submit an update to CRAN sometime later today or tomorrow.

jeffreyhanson commented 2 years ago

Just to let you know, I submitted the update yesterday and received notification that it's on it's way to CRAN.