rvalavi / blockCV

The blockCV package creates spatially or environmentally separated training and testing folds for cross-validation to provide a robust error estimation in spatially structured environments. See
https://doi.org/10.1111/2041-210X.13107
GNU General Public License v3.0
106 stars 22 forks source link

revdep check failure on blockCV with upcoming sf 1.0-0 #18

Closed edzer closed 3 years ago

edzer commented 3 years ago

See https://github.com/r-spatial/sf/issues/1649 : CRAN revdep checks flagged an error on blockCV, however I cannot reproduce this locally. Please make sure this is a false positive, or act accordingly.

pat-s commented 3 years ago

@rvalavi

This also causes subsequent errors for packages importing {blockCV}, such as {mlr3spatiotempcv}. I also cannot reproduce locally with {sf} 1.0 but I see the errors on GHA and on CRAN.

@edzer As you might know best how {s2} is used under the hood - do you have an idea if additional steps need to be taken locally to trigger the errors on CRAN? I know you said you cannot reproduce yourself but something needs to be different on CRAN which causes these errors. Maybe the issue is on CRAN's side?

edzer commented 3 years ago

If GHA and CRAN break but local checking doesn't, it's usually a dependency missing: GHA and CRAN test in environments with minimal # or packges installed.

pat-s commented 3 years ago

Which dependency would potentially be missing in this case?

Today I got an email that {mlr3spatiotempcv} is scheduled for removal because {blockCV} tests are failing - which I already expected. I can of course just remove the CRAN {blockCV} tests but the issue itself won't go away, especially not for users.

The error is

Error in st_geos_binop("intersects", x, y, sparse = sparse, prepared = prepared, :
  st_crs(x) == st_crs(y) is not TRUE
Calls: <Anonymous> ... withVisible -> suppressMessages -> withCallingHandlers

and appears on CRAN and on GHA. Caused by blockCV::spatial_block(). Haven't digged deeper yet though as this should not be my responsibility 😄

I was just wondering if you might have an idea - especially for @rvalavi - what exactly might cause this error within the {sf} 1.0 changes.

edzer commented 3 years ago

Set up an environment similar to CRAN and GHA: you could try to use tools::check_packages_in_dir() to set up a minimal environment -- I think that is what CRAN uses too.

edzer commented 3 years ago

The missing package is rgdal.

edzer commented 3 years ago

@rsbivand we have a revdep failure because

> as(st_crs(4326), "CRS")
CRS arguments: NA 

returns NA when rgdal is not installed. I think this should either pass without verifying the wkt, or break with an error.

edzer commented 3 years ago

@rvalavi if you add rgdal to Suggests: in DESCRIPTION, this issue goes away. If we submit a new sp it also will go away, but I have no E.T.A. for that.

rsbivand commented 3 years ago

I'll look at an sp error message; there is no way to see whether a WKT2 is valid without rgdal now. Should part of the deprecate rgdal/rgeos have sp optionally call back to sf?

edzer commented 3 years ago

@rsbivand let's continue the sp CRS checking discussion at https://github.com/edzer/sp/issues/107

rvalavi commented 3 years ago

Thanks everyone for checking this. @edzer I'll add rgdal to SUGGESTS and check for any other issues today and fix them if there will be any.

pat-s commented 3 years ago

Thanks for taking this on!

Appreciated if you could let us know if there is still action needed from our side to prevent CRAN removals or if you are planning to issue an update to {rgdal}/{sp} in time which might fix this 👍

rsbivand commented 3 years ago

Please do not wait for sf/sp submissions, which typically take substantial time to prepare. Am running 900 revdeps now on a system without rgdal, but missing rgdal may well throw up other problems in raster revdeps. If you need Suggests: rgdal, go rather with that now until no sp/rgdal/raster elements are involved.

Sorry, attention diverted to 226 broken packages for sp/sf revedps absent rgdal. Will re-run with rgdal present, there lately only about 20. Many of the failures are packages depending on or importing rgdal, so expected, others are unprotected Suggests or second-order. blockCV does not fail: 00check.log with sf and sp from my forks. Same for mlr2spatiotempcv: 00check.log

edzer commented 3 years ago

With the change @rvalavi suggests (putting rgdal in Suggests: of blockCV) that should be enough, afaict that should also solve the problem for mlr3spatialtempcv.

rvalavi commented 3 years ago

@edzer, I added rgdal to suggests as you recommended. I also fixed a couple of other issues. Package passed local check and rhub check for Windows and Solaris. I just submitted the update to CRAN, hopefully it will pass online test.

rvalavi commented 3 years ago

@pat-s a new version of blockCV (v2.1.4) is now up in CRAN. Do I need to do anything about the older CRAN version (v2.1.1)?