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

Remove rgeos/rgdal dependencies and update functions for sf/terra/stars #90

Open
4 tasks
silasprincipe opened this issue Nov 27, 2023 · 14 comments
Open
4 tasks

Comments

@silasprincipe
Copy link
Contributor

silasprincipe commented Nov 27, 2023

rgeos and rgdal will be retired later this year (in fact, rgeos is already down). obistools have some dependencies on these packages that will make it non functional soon (see lines 40/41 of NAMESPACE).

Suggestions:

  • Convert dependency on rgeos::gDistance to sf::st_distance (to check)
  • Convert dependency on rgeos::readWKT to sf::st_read or terra::vect (to check which one is better suited)
  • Convert anything depending on sp to modern sf
  • Convert anything depending on raster to modern and faster terra

This may include a revision of obistools to check if any function deserves an update or deprecation.

@reblake
Copy link

reblake commented Dec 15, 2023

I ran into this dependency problem today, as I'm trying to do the exercises in Module 6 of the Contributing and publishing datasets to OBIS Course in Ocean Teacher. Now I'm stuck, as the course instructs you to use obistools, but I can't install obistools because I can't install rgeos. Is there any way this issue could be elevated in priority so folks taking the course can continue on with it? I'm sure I'm not the only one.

@pieterprovoost pieterprovoost self-assigned this Dec 15, 2023
@pieterprovoost
Copy link
Member

Thanks, we'll fix this asap.

@pieterprovoost
Copy link
Member

@pieterprovoost
Copy link
Member

@silasprincipe Do you have some time to work on the raster issue at https://github.com/iobis/obistools/blob/master/tests/testthat/test_check_depth.R#L83? Alternatively we can remove the custom bathymetry functionality for now. @rubenpp7, do any of your tools use the bathymetry parameter in check_depth()?

@pieterprovoost
Copy link
Member

pieterprovoost commented Dec 16, 2023

If anyone wants to test, please try:

devtools::install_github("iobis/obistools@fix-spatial")

Note that outlier detection is currently not working, and the use of external bathymetry layers has been disabled.

The package still depends on sp and raster due to the Leaflet dependency. See:

pkgnet::CreatePackageReport(pkg_name = "obistools")

@silasprincipe
Copy link
Contributor Author

Hi @pieterprovoost,

I restored the custom bathymetry to use only terra functions. One thing that I was wondering if it would not be good to warn the user that depth in OBIS is expressed always as positive numbers for below surface (that is, something is 5m below water, and not -5). Sometimes people use bathymetry in negative terms, and supplying an all negative bathymetry layer would result all points raising a warning.

Also did some work on the calculate_centroid function to use only terra and not a mix sf + terra. Note that for now I added one workaround for MULTIPOINT type, but I'm contacting terra maintainer to see if there is a more elegant way of solving that.

Passing all tests through devtools::test(), except this one:

test_that("calculate_centroid accross dateline works", {
  centr <- calculate_centroid("POLYGON ((179 -1, 179 1, -179 1, -179 -1, 179 -1))")
  expect_equal(abs(centr[1,1]), 180)
  expect_equal(centr[1,2], 0)
})

But the previous version (mix sf+terra) was also failing at this one. Not sure if this is something we should bother for now.

Dependency from terra and sp are just because of leaflet now, and I suppose they will soon remove those dependencies.

@rubenpp7
Copy link

Hey @pieterprovoost and @silasprincipe ,

Thanks a lot for working on this.
I am not using the bathymetry parameter in check_depth

@silasprincipe
Copy link
Contributor Author

@pieterprovoost performed tests on Windows today. Had to add mode = "wb" when downloading the land shape for check_onland. Except for that, all the rest is working (tests only fail on the already mentioned test). I think the branch is ready to go.

@reblake
Copy link

reblake commented Dec 20, 2023

Thanks all for your prompt work on this! I was able to use devtools::install_github("iobis/obistools@fix-spatial") to install obistools and successfully do almost everything in the instructional video for this module of the course. The exception was the two mapping functions plot_map() and plot_map_leaflet(), which produced errors or weird images. I'm not sure if that's at all related to this issue though, and I've found a workaround using ggplot so it's not a blocker for me. Thanks again!

@silasprincipe
Copy link
Contributor Author

@reblake thanks for the feedback! I tested here and both the plot_map() and plot_map_leaflet() functions are working properly. Can you provide a few more details about when specifically the functions are producing errors (and also, which are those errors)? If possible, send also the result from sessionInfo().

@pieterprovoost I did one more commit: I removed the full import of the terra package to avoid the startup conflict messages. terra (as its predecessor raster) have several conflicts with packages from the tidyverse. So, its better for us to never import it in full on obistools, and always use terra::fun()

@reblake
Copy link

reblake commented Dec 21, 2023

@silasprincipe , I've put an example that should reproduce the error and weird plot from the mapping functions (it does on my machine) in this gist rather than clogging up this issue. I've also included my sessionInfo() and a screenshot of the output on my machine. Again, this isn't a blocker for me, and it's not related to this issue, so happy to make a new issue if it is needed. Thanks.

@pieterprovoost
Copy link
Member

@reblake Regarding the gist, is it possible that longitude and latitude are switched?

@silasprincipe
Copy link
Contributor Author

@pieterprovoost yes, this is the problem - I just answered you @reblake on your gist about that, check the solution there.

@reblake
Copy link

reblake commented Jan 8, 2024

Thanks @silasprincipe. My manual plotting worked exactly correctly, so I didn't think this was the problem.
Any chance you could add an informative error message to the two plotting functions in obistools? That would be helpful in giving package users a place to start troubleshooting plotting issues.

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

No branches or pull requests

4 participants