Implement flow mode

[SarahAlidoost]

closes #4

🔴 This branch is based on fix_5 branch. First PR #19 should be merged.

[fnattino]

Thanks for the speedy implementation, amazing! I have basically only one question/comment - I feel we are very close in getting this working already!

[SarahAlidoost]

Thanks for the speedy implementation, amazing! I have basically only one question/comment - I feel we are very close in getting this working already!

Thanks, I addressed your comments, please let me know if something else needs to be improved.

[fnattino]

Amazing @SarahAlidoost, thanks a lot!

I have tested it, and it works great. Also tested on the real data set, and it seems to work fine as well! When the real data gets added to the package as part of #2, we can then add a test that checks the behaviour as part of CI (I have raised an issue on this topic to remember, see #24). Anyway, not something we should address as part of this PR.

Just one question: when checking input we now have the following conditions:

  if (attributes && !flow_mode) {
    stop("Stroke attributes can be returned only if `flow_mode = TRUE`)")
  }

if (!is.null(from_edge) && (attributes || flow_mode)) {
    stop("from_edge is not compatible with attributes or flow_mode")

I remember we discussed the second one together and I think I suggested that flow_mode and from_edge should be exclusive - but I am now wondering whether this should actually be the case. flow_mode only acts on the selection of best_links and from_edge only acts on merge_lines, so maybe one can actually use these together?

Maybe only from_edge and attribute should exclude each other, since from_edge allows for a segment (or edge) to be reused as part of multiple strokes, so you would not know which stroke label to assign to each edge. For the same reason, attribute mode could only be used together with flow_mode.

So maybe we should leave the first condition untouched:

  if (attributes && !flow_mode) {
    stop("Stroke attributes can be returned only if `flow_mode = TRUE`)")
  }

but modify the second to allow for from_edge and flow_mode together?

if (!is.null(from_edge) && attributes) {
    stop("`from_edge` is not compatible with `attributes = TRUE`")

What do you think?

[SarahAlidoost]

I think from_edge and flow_mode have some incompatibility issues. In from_edge, we allow a segment to repeat in multiple strokes, but in flow_mode we dont allow the edge to be split. See the test below. I am not sure if the actual is expected because P1 isnot a part of edge 2.

test_that("set from_edge to one value and flow_mode is true", {
  new_l1 <- sf::st_linestring(c(p1, p2, p5))
  sfc <- sf::st_sfc(new_l1, l2)
  # p1 - p2 - p3
  #         \
  #           p5
  expected <-  sf::st_sfc(l2)
  actual <- stroke(sfc, flow_mode = TRUE, from_edge = c(2))
  expect_setequal(actual, expected)
})

and also another test with two edges

test_that("set from_edge to a list and flow_mode is true", {
  new_l1 <- sf::st_linestring(c(p1, p2, p3))
  sfc <- sf::st_sfc(new_l1, l4, l7)
  # p1 - p2 - p3
  #         \
  #           p5 - p6
  stroke_1 <- sf::st_linestring(c(p1, p2, p3))
  stroke_2 <- sf::st_linestring(c(p5, p6))
  expected <- sf::st_sfc(stroke_1, stroke_2)
  actual <- stroke(sfc, flow_mode = TRUE, from_edge = list(1, 3))
  expect_setequal(actual, expected)
})

The tests above have failed. So, I noticed that from_edge does split the edge which should not be the case if flow_mode = TRUE.

[fnattino]

You are definitely right @SarahAlidoost , from_edge will not necessarily "respect" the original edges - so we need to leave the constraints on input as they are. Please go ahead and merge! :)