UrbanAnalyst / dodgr

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

dodgr_to_igraph silently drops fields #249

Open agila5 opened 2 months ago

agila5 commented 2 months ago

See reprex below:

library(dodgr)
tmp <- hampi |> weight_streetnet() |> dodgr_centrality()
colnames(tmp)  
#>  [1] "geom_num"      "edge_id"       "from_id"       "from_lon"     
#>  [5] "from_lat"      "to_id"         "to_lon"        "to_lat"       
#>  [9] "d"             "d_weighted"    "highway"       "way_id"       
#> [13] "component"     "bicycle"       "time"          "time_weighted"
#> [17] "centrality"

# Now convert to igraph structure
library(igraph)
dodgr_to_igraph(tmp) |> edge_attr() |> names()
#> [1] "weight"        "d_weighted"    "time"          "time_weighted"
#> [5] "xfr"           "yfr"           "xto"           "yto"          
#> [9] "component"

Created on 2024-09-24 with reprex v2.0.2

As we can see, the centrality column disappears when converting dodgr to igraph. AFAICT, the column is dropped here:

https://github.com/UrbanAnalyst/dodgr/blob/ac1eec58a947eab2c89fb04a600b8417ec6f3460/R/graph-conversion.R#L158

Happy to create a PR but I'm not sure what is the ideal API. Maybe a new argument that permits the addition of "extra" column in the edge_attribute field?

mpadge commented 1 month ago

good point @agila5. The original idea with that was that the igraph version would only inherit the columns returned by the (internal) function dodgr:::dodgr_graph_cols(). All the actual machinery of dodgr only works on those columns, with lots of extra code to hold extra cols aside and then re-attach them later. Given that, i guess the ideal way would be a keep_cols parameter like in the weight streetnet functions. What do you think?