UrbanAnalyst / dodgr

Distances on Directed Graphs in R
https://urbananalyst.github.io/dodgr/
127 stars 16 forks source link

Error with dodgr_to_sf after running dodgr_contract_graph #155

Closed agila5 closed 3 years ago

agila5 commented 3 years ago

Hi @mpadge! I was testing some dodgr code for contracting a road network and I think I found a bug in dodgr_to_sf() or dodgr_contract_graph(). Reprex for the error:

# packages
library(sf)
#> Linking to GEOS 3.8.0, GDAL 3.0.4, PROJ 6.3.1
library(dodgr)

# fake data
fake_sf <- st_sf(
  data.frame(ID = 1:3, highway = c("primary", "secondary", "secondary")), 
  geometry = st_sfc(
    st_linestring(rbind(c(0, 0), c(0.01, 0.01))),
    st_linestring(rbind(c(0.01, 0.01), c(0.02, 0.02))), 
    st_linestring(rbind(c(0.02, 0.02), c(0.03, 0.03))),
    crs = 4326
  ) 
)

# Everything is fine
weight_streetnet(fake_sf) %>% 
  dodgr_to_sf()
#> Using column ID as ID column for edges; please specify explicitly if a different column should be used.
#> Simple feature collection with 4 features and 15 fields
#> geometry type:  LINESTRING
#> dimension:      XY
#> bbox:           xmin: 0 ymin: 0 xmax: 0.03 ymax: 0.03
#> geographic CRS: WGS 84
#>   geom_num edge_id from_id from_lon from_lat to_id to_lon to_lat        d
#> 1        2 a123459       3     0.03     0.03     1   0.01   0.01 3148.591
#> 2        2 a123460       1     0.01     0.01     3   0.03   0.03 3148.591
#> 3        1       1       0     0.00     0.00     1   0.01   0.01 1574.295
#> 4        1       2       1     0.01     0.01     0   0.00   0.00 1574.295
#>   d_weighted   highway way_id     time time_weighted component
#> 1   3935.738 secondary      2 755.6617      944.5772         1
#> 2   3935.738 secondary      2 755.6617      944.5772         1
#> 3   2248.993   primary      1 377.8309      539.7584         1
#> 4   2248.993   primary      1 377.8309      539.7584         1
#>                         geometry
#> 1 LINESTRING (0.03 0.03, 0.02...
#> 2 LINESTRING (0.01 0.01, 0.02...
#> 3    LINESTRING (0 0, 0.01 0.01)
#> 4    LINESTRING (0.01 0.01, 0 0)

# Error
weight_streetnet(fake_sf) %>% 
  dodgr_contract_graph() %>% 
  dodgr_to_sf()
#> Using column ID as ID column for edges; please specify explicitly if a different column should be used.
#> Error in rcpp_aggregate_to_sf(graph, gc, edge_map): CHAR() can only be applied to a 'CHARSXP', not a 'NULL'

# Same error with intermediate object and running clear_dodgr_cache
temp_net <- weight_streetnet(fake_sf) %>% 
  dodgr_contract_graph()
#> Using column ID as ID column for edges; please specify explicitly if a different column should be used.
clear_dodgr_cache()
dodgr_to_sf(temp_net)
#> Error in rcpp_aggregate_to_sf(graph, gc, edge_map): CHAR() can only be applied to a 'CHARSXP', not a 'NULL'

Created on 2021-02-11 by the reprex package (v0.3.0)

I'm not 100% sure what's going on since, I think, the contraction step is automatically applied when running dodgr_to_sf().

Session info ``` r devtools::session_info() #> - Session info --------------------------------------------------------------- #> setting value #> version R version 3.6.3 (2020-02-29) #> os Windows 10 x64 #> system x86_64, mingw32 #> ui RTerm #> language (EN) #> collate Italian_Italy.1252 #> ctype Italian_Italy.1252 #> tz Europe/Berlin #> date 2021-02-11 #> #> - Packages ------------------------------------------------------------------- #> ! package * version date lib source #> assertthat 0.2.1 2019-03-21 [1] CRAN (R 3.6.0) #> callr 3.5.1 2020-10-13 [1] CRAN (R 3.6.3) #> class 7.3-17 2020-04-26 [1] CRAN (R 3.6.3) #> classInt 0.4-3 2020-04-07 [1] CRAN (R 3.6.3) #> cli 2.3.0 2021-01-31 [1] CRAN (R 3.6.3) #> crayon 1.4.1 2021-02-08 [1] CRAN (R 3.6.3) #> curl 4.3 2019-12-02 [1] CRAN (R 3.6.1) #> DBI 1.1.1 2021-01-15 [1] CRAN (R 3.6.3) #> desc 1.2.0 2018-05-01 [1] CRAN (R 3.6.0) #> devtools 2.3.2 2020-09-18 [1] CRAN (R 3.6.3) #> digest 0.6.27 2020-10-24 [1] CRAN (R 3.6.3) #> dodgr * 0.2.8 2021-01-31 [1] CRAN (R 3.6.3) #> dplyr 1.0.4 2021-02-02 [1] CRAN (R 3.6.3) #> e1071 1.7-4 2020-10-14 [1] CRAN (R 3.6.3) #> ellipsis 0.3.1 2020-05-15 [1] CRAN (R 3.6.3) #> evaluate 0.14 2019-05-28 [1] CRAN (R 3.6.0) #> fs 1.5.0 2020-07-31 [1] CRAN (R 3.6.3) #> generics 0.1.0 2020-10-31 [1] CRAN (R 3.6.3) #> glue 1.4.2 2020-08-27 [1] CRAN (R 3.6.3) #> highr 0.8 2019-03-20 [1] CRAN (R 3.6.0) #> htmltools 0.5.0 2020-06-16 [1] CRAN (R 3.6.3) #> httr 1.4.2 2020-07-20 [1] CRAN (R 3.6.3) #> jsonlite 1.7.1 2020-09-07 [1] CRAN (R 3.6.3) #> KernSmooth 2.23-18 2020-10-29 [1] CRAN (R 3.6.3) #> knitr 1.30 2020-09-22 [1] CRAN (R 3.6.3) #> lattice 0.20-41 2020-04-02 [1] CRAN (R 3.6.3) #> lifecycle 0.2.0 2020-03-06 [1] CRAN (R 3.6.2) #> lubridate 1.7.9.2 2020-11-13 [1] CRAN (R 3.6.3) #> magrittr 2.0.1 2020-11-17 [1] CRAN (R 3.6.3) #> memoise 1.1.0 2017-04-21 [1] CRAN (R 3.6.0) #> osmdata 0.1.4 2020-11-04 [1] CRAN (R 3.6.3) #> pillar 1.4.7 2020-11-20 [1] CRAN (R 3.6.3) #> pkgbuild 1.1.0 2020-07-13 [1] CRAN (R 3.6.3) #> pkgconfig 2.0.3 2019-09-22 [1] CRAN (R 3.6.1) #> pkgload 1.1.0 2020-05-29 [1] CRAN (R 3.6.3) #> prettyunits 1.1.1 2020-01-24 [1] CRAN (R 3.6.2) #> processx 3.4.4 2020-09-03 [1] CRAN (R 3.6.3) #> ps 1.4.0 2020-10-07 [1] CRAN (R 3.6.3) #> purrr 0.3.4 2020-04-17 [1] CRAN (R 3.6.3) #> R6 2.5.0 2020-10-28 [1] CRAN (R 3.6.3) #> Rcpp 1.0.6 2021-01-15 [1] CRAN (R 3.6.3) #> D RcppParallel 5.0.2 2020-06-24 [1] CRAN (R 3.6.3) #> remotes 2.2.0 2020-07-21 [1] CRAN (R 3.6.3) #> rlang 0.4.10 2020-12-30 [1] CRAN (R 3.6.3) #> rmarkdown 2.5 2020-10-21 [1] CRAN (R 3.6.3) #> rprojroot 2.0.2 2020-11-15 [1] CRAN (R 3.6.3) #> rvest 0.3.6 2020-07-25 [1] CRAN (R 3.6.3) #> sessioninfo 1.1.1 2018-11-05 [1] CRAN (R 3.6.0) #> sf * 0.9-7 2021-01-06 [1] CRAN (R 3.6.3) #> sp 1.4-4 2020-10-07 [1] CRAN (R 3.6.3) #> stringi 1.5.3 2020-09-09 [1] CRAN (R 3.6.3) #> stringr 1.4.0 2019-02-10 [1] CRAN (R 3.6.0) #> testthat 3.0.0 2020-10-31 [1] CRAN (R 3.6.3) #> tibble 3.0.6 2021-01-29 [1] CRAN (R 3.6.3) #> tidyselect 1.1.0 2020-05-11 [1] CRAN (R 3.6.3) #> units 0.6-7 2020-06-13 [1] CRAN (R 3.6.3) #> usethis 2.0.0 2020-12-10 [1] CRAN (R 3.6.3) #> vctrs 0.3.6 2020-12-17 [1] CRAN (R 3.6.3) #> withr 2.3.0 2020-09-22 [1] CRAN (R 3.6.3) #> xfun 0.19 2020-10-30 [1] CRAN (R 3.6.3) #> xml2 1.3.2 2020-04-23 [1] CRAN (R 3.6.3) #> yaml 2.2.1 2020-02-01 [1] CRAN (R 3.6.2) #> #> [1] C:/Users/Utente/Documents/R/win-library/3.6 #> [2] C:/Program Files/R/R-3.6.3/library #> #> D -- DLL MD5 mismatch, broken installation. ```
mpadge commented 3 years ago

Thanks as always @agila5, definitely a bug, but one that occurred because dodgr_to_sf() is not supposed to be called on contracted graphs. That commit nevertheless allows it, because I suppose it may sometimes be useful, but generates the following:

library(sf)
#> Linking to GEOS 3.8.1, GDAL 3.0.4, PROJ 6.3.2
library (dodgr)
packageVersion ("dodgr")
#>[1] '0.2.8.2'

# fake data
fake_sf <- st_sf(
  data.frame(ID = 1:3, highway = c("primary", "secondary", "secondary")), 
  geometry = st_sfc(
    st_linestring(rbind(c(0, 0), c(0.01, 0.01))),
    st_linestring(rbind(c(0.01, 0.01), c(0.02, 0.02))), 
    st_linestring(rbind(c(0.02, 0.02), c(0.03, 0.03))),
    crs = 4326
  ) 
)

weight_streetnet(fake_sf) %>% 
  dodgr_contract_graph() %>% 
  dodgr_to_sf()
#> Using column ID as ID column for edges; please specify explicitly if a different column should be used.
#> 'dodgr_to_sf' is intended to be called only on non-contracted graphs.
#> Calling on a contracted graph will result in loss of information.
#> Simple feature collection with 4 features and 15 fields
#> geometry type:  LINESTRING
#> dimension:      XY
#> bbox:           xmin: 0 ymin: 0 xmax: 0.03 ymax: 0.03
#> geographic CRS: WGS 84
#>   geom_num edge_id from_id from_lon from_lat to_id to_lon to_lat        d
#> 1        2       1       1     0.01     0.01     3   0.03   0.03 3148.591
#> 2        2       2       3     0.03     0.03     1   0.01   0.01 3148.591
#> 3        1       3       0     0.00     0.00     1   0.01   0.01 1574.295
#> 4        1       4       1     0.01     0.01     0   0.00   0.00 1574.295
#>   d_weighted   highway way_id     time time_weighted component
#> 1   3935.738 secondary      2 755.6617      944.5772         1
#> 2   3935.738 secondary      2 755.6617      944.5772         1
#> 3   2248.993   primary      1 377.8309      539.7584         1
#> 4   2248.993   primary      1 377.8309      539.7584         1
#>                         geometry
#> 1 LINESTRING (0.01 0.01, 0.03...
#> 2 LINESTRING (0.03 0.03, 0.01...
#> 3    LINESTRING (0 0, 0.01 0.01)
#> 4    LINESTRING (0.01 0.01, 0 0)

Created on 2021-02-11 by the reprex package (v1.0.0)

The key is the first two lines of output, which I've put as a message, but I suppose could also be a warning? What do you think? Otherwise the :bug: was pretty straightforward to resolve.

agila5 commented 3 years ago

Thanks for the quick fix!

The key is the first two lines of output, which I've put as a message, but I suppose could also be a warning? What do you think?

I would raise a warning message since, IMO, it's pretty easy to miss that message when there is a lot of output. Just one more question. Why do you say that it result in loss of information ?

mpadge commented 3 years ago

Why do you say that it result in loss of information ?

Because the function is intended to rebuild complete linestring objects even when you've been working on contracted graphs the whole time. It does the contraction behind the scenes, uses a cached version of the information necessary to rebuild the full graph from the contracted version, and copies all information from the contracted graph, including potentially extra columns like centrality or flow aggregation or whatever, back on to the expanded edges, so the resultant sf version should have the full original spatial detail. If you pass a contracted version, the sf version will only have straight lines between pairs of vertices representing the network junctions only, so you will lose all of the detail of line geometry between junctions.

And I'll now re-open to take your advice and issue a warning instead of message.

agila5 commented 3 years ago

Hi @mpadge. First of all, I'm sorry that I never answer your comment but that you very much for the explanation. I understand your point. I'm not sure if you want to add something here, but, if you want, I can change that message into a warning with a PR.

mpadge commented 3 years ago

Sure, that'd be great @agila5. please do!