luukvdmeer / sfnetworks

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

Spatstat error in CRAN checks #265

Closed luukvdmeer closed 6 months ago

luukvdmeer commented 6 months ago

Got an email from CRAN about an error in the checks for sfnetworks. Seems to be related to the linnet integration. @agila5 could you take a look at this when you have time? We have until 2024-04-09 to fix it.

https://cran.r-project.org/web/checks/check_results_sfnetworks.html

agila5 commented 6 months ago

Unfortunately, for the time being, I cannot replicate the error... Tomorrow I will try again!

agila5 commented 6 months ago

I'm pretty sure that the problem lies in the following lines of code

https://github.com/luukvdmeer/sfnetworks/blob/23a4125030178f5c42ebb48d2547052a31a3cdea/R/spatstat.R#L17-L18

where we are comparing the object spts_ver with the number 2.0-0. More precisely, the object spts_ver created by packageVersion("spatstat") is an object of class c("package_version", "numeric_version") and comparisons with other objects are performed by Ops.numeric_version

Ops.numeric_version
#> function (e1, e2) 
#> {
#>     if (nargs() == 1L) 
#>         stop(gettextf("unary '%s' not defined for \"numeric_version\" objects", 
#>             .Generic), domain = NA)
#>     boolean <- switch(.Generic, `<` = , `>` = , `==` = , `!=` = , 
#>         `<=` = , `>=` = TRUE, FALSE)
#>     if (!boolean) 
#>         stop(gettextf("'%s' not defined for \"numeric_version\" objects", 
#>             .Generic), domain = NA)
#>     if (!is.numeric_version(e1)) 
#>         e1 <- as.numeric_version(e1)
#>     if (!is.numeric_version(e2)) 
#>         e2 <- as.numeric_version(e2)
#>     op <- get(.Generic, mode = "function")
#>     op(.Internal(compareNumericVersion(e1, e2)), 0L)
#> }
#> <bytecode: 0x00000210253d3f10>
#> <environment: namespace:base>

Created on 2024-03-28 with reprex v2.0.2

According to the traceback reported on CRAN, this function runs smoothly until it computes base::as.numeric_version(e2) (where e2 is equal to 2 - 0 as we can read from the previous line in the traceback). The function base::as.numeric_version() internally calls base::numeric_version() which internally calls .make_numeric_version(). The function .make_numeric_version() was recently modified by the R-core team (see https://github.com/wch/r-source/commit/1338a95618ddcc8a0af77dc06e4018625de06ec3 and https://bugs.r-project.org/show_bug.cgi?id=18548) and, even more recently, they decided to always raise an error on invalid numeric version, see https://github.com/wch/r-source/commit/7b523724f74888bafdcd6866aa4ffc1d4c24c059. Therefore, IMO, we should simply modify 2.0-0 to "2.0-0" (which is probably the code always intended when comparing version numbers).

I'm 99.99% sure that the aforementioned explanation is correct. However:

  1. I cannot replicate the aforementioned error using the GHA. Would you mind checking the R-CMD-check.yaml file in the develop branch?
  2. I just noticed that we do not have any explicit dependency on spatstat (as a consequence of 0c551e47c3446ee001edaa26acf084ff672aafb9). Therefore, I don't really understand how it is possible that try(packageVersion("spatstat"), silent = TRUE) returns a valid numeric_version object (instead of an object of class try-error) and, as a consequence, I don't understand how is it possible that R ran the second part of the following if-clause during the CRAN checks detecting our error: https://github.com/luukvdmeer/sfnetworks/blob/23a4125030178f5c42ebb48d2547052a31a3cdea/R/spatstat.R#L17-L18 Any idea? Please notice that we are currently suggesting spatstat.geom and spatstat.linnet, but they do not have an explicit dependency on spatstat. In fact:
pak::pkg_deps_explain("sfnetworks", "spatstat.geom", dependencies = TRUE)
#> ℹ Loading metadata database✔ Loading metadata database ... done
#> sfnetworks -> spatstat.geom
#> sfnetworks -> spatstat.linnet -> spatstat.geom
#> sfnetworks -> spatstat.linnet -> spatstat.random -> spatstat.geom
#> sfnetworks -> spatstat.linnet -> spatstat.explore -> spatstat.geom
#> sfnetworks -> spatstat.linnet -> spatstat.explore -> spatstat.random ->
#>   spatstat.geom
#> sfnetworks -> spatstat.linnet -> spatstat.model -> spatstat.geom
#> sfnetworks -> spatstat.linnet -> spatstat.model -> spatstat.random ->
#>   spatstat.geom
#> sfnetworks -> spatstat.linnet -> spatstat.model -> spatstat.explore ->
#>   spatstat.geom
#> sfnetworks -> spatstat.linnet -> spatstat.model -> spatstat.explore ->
#>   spatstat.random -> spatstat.geom
pak::pkg_deps_explain("sfnetworks", "spatstat.linnet", dependencies = TRUE)
#> sfnetworks -> spatstat.linnet
pak::pkg_deps_explain("sfnetworks", "spatstat", dependencies = TRUE)
#> x spatstat

Created on 2024-03-28 with reprex v2.0.2

agila5 commented 6 months ago

Answers:

  1. I'm pretty sure that I correctly identified the error and I will prepare a PR in a few minutes
  2. Apparently my assumptions were incorrect, meaning that R CMD check can still "access" the packages in the library even if they are not specified under Imports or Suggests
agila5 commented 6 months ago

Hi @luukvdmeer. Friendly remainder that the deadline for the resubmission is 2024-04-09!

luukvdmeer commented 6 months ago

Yes, I am aware, I'm looking at it today, thanks!

loreabad6 commented 6 months ago

Thank you for this! sfnetworks 0.6.4. is on CRAN 😄