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

No longer import sp by default #942

Merged
merged 12 commits into from
Mar 3, 2025
Merged

No longer import sp by default #942

merged 12 commits into from
Mar 3, 2025

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Feb 28, 2025

The goal of this PR is to reduce the number of imports by removing the sp package eventually.

Now that sp imports sf and raster imports terra, it would be great if leaflet normalized objects to terra.

Before this PR, an sp object would get normalized inside leaflet, but since sp uses sf under the hood, it is better to just convert to sf in leaflet, so that it can use a common leaflet logic and leave the normalization to sf instead of having custom methods in leaflet.

I plan to replace raster usage by terra, but I will wait to see if there is appetite in leaflet for that kind of change.

Since raster and sp objects are more and more obsolete, it seems odd that leaflet continues to import these packages unconditionnally as they are used less and less nowadays.

@schloerke
Copy link
Contributor

Given #928 and {sf} is the new version of {sp}, thank you!


I plan to replace raster usage by terra, but I will wait to see if there is appetite in leaflet for that kind of change.

From https://github.com/rspatial/terra

terra replaces the raster package. The interfaces of terra and raster are similar, but terra is simpler, faster and can do more.

It's a welcome change!

@olivroy
Copy link
Contributor Author

olivroy commented Mar 3, 2025

Thanks for the quick review @schloerke ! I addressed your comments

I reverted everything related to terra / raster as I plan to take this on in a future PR.

  • I added more docs about this in the leaflet() docs
  • To be clear, conversion from sp::Spatial* objects will generally succeed unless this is an invalid shape?
  • I improved the warning to be signaled once and to include the srcref with rlang.
  • I left the S3 methods but added comments to mark them as obsolete / unused

@schloerke
Copy link
Contributor

Thank you @olivroy !

(I'll merge once the checks pass. Thank you for the separation of PRs regarding raster/terra)

@schloerke schloerke merged commit a609a34 into rstudio:main Mar 3, 2025
12 checks passed
@olivroy olivroy deleted the sp-dep branch March 3, 2025 18:48
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.

2 participants