luukvdmeer / sfnetworks

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

Make to_spatial_smooth take a merging function, like to_spatial_simple? #179

Closed Anaphory closed 2 years ago

Anaphory commented 2 years ago

Is your feature request related to a problem? Please describe.

I start with a network and then try to turn it into a simple, smooth network. This needs to be done iteratively: Imagine a network section like this.

   \|/
    1
   / \
  2   3
  |  / \\
  4 5   6
   \| //
    7
   /|\

Only smoothing it woud remove 2,4,5; only simplifying it would remove the edges around 6; but iteratively smoothing then simplifying it will remove all down to a 1-7 connection. For shortest path routing, I don't need all those edges, just the shortest connection.

However, while I can tell to_spatial_simple how to merge attributes (summarise_attributes = "first"), to_spatial_smooth has no such optional argument. Instead, I get NAs in my edge attributes.

Describe the solution you'd like

to_spatial_smooth(
    x,
    summarise_attributes = "first",
    store_original_data = FALSE)

with the same semantics for summarise_attributes as in to_spatial_simple.

Describe alternatives you've considered

I could just not smooth my graph. I start with 24951 nodes and 29292 edges and end up with 19920 nodes and 23980 edges, so it's quite a chunk.

PS: Very minor thing: While trying to understand how this could be implemented, and why it's not happening at the moment, I noticed that this check https://github.com/luukvdmeer/sfnetworks/blob/10b009e9d112d50c4f95c326b92864c1eb4f6ba6/R/morphers.R#L589 happens in every row for lapply. I think you could assume that the graph doesn't change directedness inside the lapply, and wrap the two different find_edges functions into the two different branches of that if?

Anaphory commented 2 years ago

Minimal example:

> minimal_network = sfnetwork(tibble(x = c(1,2,3), y=c(2,1,3)) %>% st_as_sf(coords=c("x", "y")), edges=tibble(from=c(1,2), to=c(2,3), attribute=c("a", "b")))
Checking if spatial network structure is valid...
Spatial network structure is valid
> minimal_network
# A sfnetwork with 3 nodes and 2 edges
#
# CRS:  NA 
#
# A rooted tree with spatially implicit edges
#
# Node Data:     3 × 1 (active)
# Geometry type: POINT
# Dimension:     XY
# Bounding box:  xmin: 1 ymin: 1 xmax: 3 ymax: 3
  geometry
   <POINT>
1    (1 2)
2    (2 1)
3    (3 3)
#
# Edge Data: 2 × 3
   from    to attribute
  <int> <int> <chr>    
1     1     2 a        
2     2     3 b        

> convert(minimal_network, to_spatial_smooth)
# A sfnetwork with 2 nodes and 1 edges
#
# CRS:  NA 
#
# A rooted tree with spatially implicit edges
#
# Node Data:     2 × 2 (active)
# Geometry type: POINT
# Dimension:     XY
# Bounding box:  xmin: 1 ymin: 2 xmax: 3 ymax: 3
  geometry .tidygraph_node_index
   <POINT>                 <int>
1    (1 2)                     1
2    (3 3)                     3
#
# Edge Data: 1 × 4
   from    to attribute .tidygraph_edge_index
  <int> <int> <chr>     <named list>         
1     1     2 NA        <dbl [2]>
agila5 commented 2 years ago

Hi @Anaphory, and thanks for raising this issue. I repeat here the same message as in #181, i.e. I'm looking forward to implementing these new features, but I cannot focus on sfnetworks at the moment. Maybe you can get inspiration from the existing code in to_spatial_smooth. Check also related issues #124 + its PR and #181.

Anaphory commented 2 years ago

Sounds good to me! I know how impossible it is to find time to do this kind of things, but I still like to leave an issue as a note to future us. I left my comment in #124 and you connected this one with #181 already, so from my end it would make sense to close this #179 – or at least maybe after someone has sanity checked this small observation and decided whether it's to be ignored, fixed soon, or filed as issue as a future reminder.

PS: Very minor thing: While trying to understand how this could be implemented, and why it's not happening at the moment, I noticed that this check https://github.com/luukvdmeer/sfnetworks/blob/10b009e9d112d50c4f95c326b92864c1eb4f6ba6/R/morphers.R#L589 happens in every row for lapply. I think you could assume that the graph doesn't change directedness inside the lapply, and wrap the two different find_edges functions into the two different branches of that if?

luukvdmeer commented 2 years ago

Thanks for raising this!

It is a heavily requested feature it seems, so we need to work on it! The reasons why this argument is right now present in e.g. to_spatial_simple() but not in to_spatial_smooth() is that to_spatial_simple() is just a wrapper around igraph::simplify(), which has the possibility to summarise attributes with the edge.attr.comb, see here. Hence, we just rely on igraph to take care of this. However, to_spatial_smooth is completely self-implemented because there is no igraph function doing the same (as far as I know). Therefore, we also need to find a way to implement the summarise_attributes functionality by ourselves (possibly based on how igraph is handling this?).

There are now multiple issues requesting the same feature. To keep the overview I will only leave #120 open and link to this issue from there.

I will also create an overview issue for all tasks we have for to_spatial_smooth(), and include your additonal comment there, thanks!