ropensci / rnaturalearth

An R package to hold and facilitate interaction with natural earth map data :earth_africa:
http://ropensci.github.io/rnaturalearth/
Other
214 stars 24 forks source link

Patch raster -> terra dependency changes #63

Closed khufkens closed 1 year ago

khufkens commented 1 year ago

This patches the raster dependency as raised by #60

Follow up will deal with the rgdal, which is more challenging. rgdal deals with reading in shape files, and although this can be replaced by sf the back conversions to sp for consistency are tedious due sp, well, not being sf.

defuneste commented 1 year ago

Thx for doing it! Do you know what kind of format are available in https://www.naturalearthdata.com/features/ ?

sf::read_sf() can handle a lot but not all. {sf} is already in Imports (should also probably up the minimum version required). Then instead of converting to sf after cleaning -99& co we could do the reverse and convert to {sp} if needed ?

khufkens commented 1 year ago

This is what I have partially implemented but the back conversions break on some of the features, or are non-trivial. Types need to be correctly specified as there is no catch all as st_as_sf() as it is currently specified for going from sp to sf.

defuneste commented 1 year ago

Yeah ... Can as(x, "Spatial") be more specific and works better?

khufkens commented 1 year ago

Thanks, this works. Build tests on my local machine are clean but I'm not sure how comprehensive unit tests are.

andysouth commented 1 year ago

Many thanks for your work @khufkens @defuneste @maelle Apologies for my quietness. I have tested this locally and am happy to merge. Also happy to add you as contributors ? Next question that I'll put in a following issue is whether we should change default vector class from sp to sf, which seems like a good idea but could be a breaking change for those that haven't specified returnclass.

khufkens commented 1 year ago

As long as sp is still a valid option (as through the current workflow) I would not introduce breaking changes.

Would be happy to be mentioned as contributor, thanks!

Nowosad commented 1 year ago

@rsbivand, can you give us your opinion to the question of @andysouth [" whether we should change default vector class from sp to sf"]?

rsbivand commented 1 year ago

Try _SP_EVOLUTION_STATUS_=2 R CMD check as per https://r-spatial.org/r/2022/12/14/evolution2.html and https://rsbivand.github.io/csds_jan23/bivand_csds_ssg_230117.pdf. (sp needs a release @edzer - maybe with my fork or accepting my PRs) If examples and tests succeed (and after replacing use in code of rgdal:.readOGR() by as(sf::st_read(, quiet=TRUE), "Spatial")), should be OK. sp will then be using sf rather than rgdal. It will not work if sp::over() methods are used, they are being dropped. Go for a development version of sf too.

PMassicotte commented 1 year ago

@khufkens Can you provide information (full name, email) so we can add you as a contributor?

khufkens commented 1 year ago

@PMassicotte for the DESCRIPTION file

            person(
              family = "Hufkens",
              given = "Koen",
              email = "koen.hufkens@gmail.com",
              role = c("ctb"),
              comment = c(ORCID = "0000-0002-5070-8109")
              )