luukvdmeer / sfnetworks

Tidy Geospatial Networks in R
https://luukvdmeer.github.io/sfnetworks/
Other
347 stars 20 forks source link

Adapt code to spatstat v2 #138

Closed agila5 closed 3 years ago

agila5 commented 3 years ago

Fix #137.

Finally, it seems that it's working. I will complete the PR as soon as sf 0.9.8 is on CRAN (the 0.9.7 version is already failing). The only downside is that (I think) we have to add a dependency on the 0.9.8 version of sf or, at least, test the sf version in the spatstat-related code. Do you think it's a problem?

codecov-io commented 3 years ago

Codecov Report

Merging #138 (236318a) into master (4a65a02) will increase coverage by 0.00%. The diff coverage is 57.69%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #138   +/-   ##
=======================================
  Coverage   86.19%   86.20%           
=======================================
  Files          20       20           
  Lines        1326     1341   +15     
=======================================
+ Hits         1143     1156   +13     
- Misses        183      185    +2     
Impacted Files Coverage Δ
R/sfnetwork.R 86.32% <50.00%> (-1.40%) :arrow_down:
R/spatstat.R 60.00% <60.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4a65a02...236318a. Read the comment docs.

agila5 commented 3 years ago

I think it's ready to be reviewed/merged.

IMO the only problem is that it introduces a "strong" dependency on sf >= 0.9.8 (and also an ugly startup message related to spatstat.geom). Hence, I have the following proposal. I am probably the only one that takes advantage of the integration between sfnetworks and spatstat. So, maybe, we could purge all spatstat code from sfnetworks, and then I will create a new (small but with a lot of dependencies) package that extends sfnetworks. Opinions?

luukvdmeer commented 3 years ago

I am probably the only one that takes advantage of the integration between sfnetworks and spatstat

Not sure about that! I think it is great to have integration with other packages. I was planning to implement similar integration with e.g. spdep.

What is the reason we need the sf dependency? I don't see sf functions being called in your code (but might have looked over them). If we need it, I would suggest to check for the sf version in the check_spatstat function, rather than setting the depedency for the whole package.

Regarding the startup message, you refer to this one?

Registered S3 method overwritten by 'spatstat':
  method     from
  print.boxx cli 

I think we can call s3_register("spatstat.linnet::as.linnet", "sfnetwork") inside suppressPackageStartupMessages() to avoid that, but not sure if that is good practice.

agila5 commented 3 years ago

What is the reason we need the sf dependency? I don't see sf functions being called in your code (but might have looked over them).

The method for converting sf objects (with POINT geometry) into ppp objects is defined here and used here

https://github.com/luukvdmeer/sfnetworks/blob/236318a132f1d52f8bb93a00c0d6a72c155324e9/R/spatstat.R#L54-L55

The method for converting psp (and linnet) objects into sf objects is defined here and used here

https://github.com/luukvdmeer/sfnetworks/blob/236318a132f1d52f8bb93a00c0d6a72c155324e9/R/sfnetwork.R#L308-L311

The problem is that the code in the sf package version 0.9.7 fails because it calls spatstat functions that were transferred to a different package.

If we need it, I would suggest to check for the sf version in the check_spatstat function, rather than setting the depedency for the whole package.

Agree. Changed with the last commit.

Regarding the startup message, you refer to this one?

Yes

I think we can call s3_register("spatstat.linnet::as.linnet", "sfnetwork") inside suppressPackageStartupMessages() to avoid that, but not sure if that is good practice.

I added a call to suppressPackageStartupMessages(), feel free to remove that.