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

Migrate from raster to terra #39

Open
elipousson opened this issue May 11, 2023 · 6 comments
Open

Migrate from raster to terra #39

elipousson opened this issue May 11, 2023 · 6 comments

Comments

@elipousson
Copy link
Contributor

Given the encouragement by package maintainers for everyone to migrate from raster to terra, I thought it may be helpful to make this change for mapboxapi.

I already was able to replace raster functions with the terra versions for layer_static_mapbox() and tm_static_mapbox(). The only other using raster is get_static_tiles() which is a bit more complicated. It looks like ceramic (which you cite as the source for code that was adapted and incorporated into the function) has also already migrated to terra so I think it shouldn't be too difficult to figure out based on this updated code: https://github.com/hypertidy/ceramic/blob/f228ea6e5842cb55cafa413bdcb921b36047849a/R/get_tiles.R

If you wanted to tackle the latter changes to get_static_tiles() yourself, I'm happy to open a pull request for the other changes. If not, I'd be happy to figure it out sometime in the next couple weeks.

@elipousson
Copy link
Contributor Author

Happy to see #40 merged! I did want to flag that while I had started substituting {RcppSimdJson} for {jsonlite}, I didn't quite finish that change. It also might be worth looking at the new {yyjsonr} package: https://github.com/coolbutuseless/yyjsonr Either way it is low priority and not directly related to the raster to terra migration.

Still happy to revisit Michael Sumner's code in ceramic later this fall to see if I can figure out the needed changes to get_static_tiles().

@walkerke
Copy link
Owner

walkerke commented Sep 7, 2023

yeah I'm noticing some breakages there, I'm going to revert back to jsonlite.

@walkerke
Copy link
Owner

walkerke commented Sep 7, 2023

Sorry @elipousson - I noticed this PR broke several functions as I was testing so I needed to revert. Let's review piece-by-piece; I think removing jsonlite introduced several problems (particularly with the navigation functions).

@elipousson
Copy link
Contributor Author

elipousson commented Sep 8, 2023

No apologies needed—my fault for not keeping the pull request clean. Have you considered expanding the tests in testthat and adding some code coverage checks to the repository? It seems helpful for other packages I've contributed to.

@elipousson
Copy link
Contributor Author

Would you suggest opening a new pull request just focused on removing the raster dependency from static_mapbox() to start, @walkerke?

@walkerke
Copy link
Owner

walkerke commented Sep 8, 2023

Would you suggest opening a new pull request just focused on removing the raster dependency from static_mapbox() to start, @walkerke?

Yes I think that'd be good! We can then consider the JSON stuff at a later date (I've seen a few posts about issues people have had with RcppSimdJson).

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

No branches or pull requests

2 participants