Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch raster -> terra dependency changes #63

Merged
merged 7 commits into from
Jan 12, 2023
Merged

Patch raster -> terra dependency changes #63

merged 7 commits into from
Jan 12, 2023

Conversation

khufkens
Copy link
Member

@khufkens khufkens commented Jan 6, 2023

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
Copy link

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
Copy link
Member Author

khufkens commented Jan 7, 2023

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
Copy link

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

@khufkens
Copy link
Member Author

khufkens commented Jan 7, 2023

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

@maelle maelle mentioned this pull request Jan 10, 2023
@andysouth
Copy link
Contributor

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
Copy link
Member Author

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
Copy link

Nowosad commented Jan 12, 2023

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

@rsbivand
Copy link

rsbivand commented Jan 12, 2023

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
Copy link
Contributor

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

@khufkens
Copy link
Member Author

@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")
              )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants