test: merged and refactored flow.R tests #1675
Conversation
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
| is.null(target) && !is.null(source)) { | ||
| stop("Please give both source and target or neither") | ||
|
|
||
| if (xor(is.null(source), is.null(target))) { |
There was a problem hiding this comment.
OMG I did not know about xor()! 🤯
Could you explain why we should use it instead of the operator directly?
There was a problem hiding this comment.
The only reason I had to switch to it was that it is shorter and expresses what we want more clearly.
xor
#> function (x, y)
#> {
#> (x | y) & !(x & y)
#> }
#> <bytecode: 0x10697add8>
#> <environment: namespace:base>But now I am note sure if it is 100% ok to use, given that it uses single | and &? Could this be an issue? I think not
| } | ||
| } | ||
|
|
||
| #' @rdname edge_connectivity | ||
| #' @export | ||
| edge_disjoint_paths <- function(graph, source, target) { | ||
| edge_disjoint_paths <- function(graph, source = NULL, target = NULL) { |
There was a problem hiding this comment.
why change the default value especially if the code won't work with it?
There was a problem hiding this comment.
is it to align it with the function below?
There was a problem hiding this comment.
Yes this was just to align the code with the rest. But I don't mind changing it back if you think it is better to leave it untouched
| is <- sapply(msts, is_separator, graph = g_zachary) | ||
| expect_equal(unique(is), TRUE) | ||
|
|
||
| ## TODO: check that it is minimal |
There was a problem hiding this comment.
is this hard / still valid?
There was a problem hiding this comment.
library(igraph)
g_zachary <- make_graph("Zachary")
msts <- min_st_separators(g_zachary)
sapply(msts, is_min_separator, graph = g_zachary)
#> [1] TRUE TRUE TRUE FALSE FALSE FALSE FALSE FALSE FALSE FALSE FALSE FALSE
#> [13] TRUE TRUE FALSE FALSE FALSE FALSE TRUE TRUE TRUE FALSE FALSE FALSE
#> [25] FALSE FALSE FALSE FALSE TRUE TRUE TRUE TRUE FALSE FALSE FALSE FALSE
#> [37] FALSE FALSE FALSE@szhorvat not too familiar with the topic. Is the output problematic? shouldnt all be TRUE?
There was a problem hiding this comment.
I have not verified all of this but it should be correct.
This is indeed tricky and a bit confusing.
All of these are separators in the sense that their removal makes some vertex u unreachable from some vertex v.
"Minimal" in general means that we can't shrink the set while maintaining its property. We do indeed need to remove all these vertices to separate parts of the graph. But here's a quirk:
msts[[4]] is 1,3,14,20,31. You drop any of 3, 14, 20 and 31 and this would still be a separator, so it's not a "minimal separator". This is why is_min_separator() returns false. But it is a "minimal s-t separator" because there exists a vertex pair which can only be separated by removing all these vertices. An example is 2 and 33.
See the docs here for an example: https://igraph.org/c/html/latest/igraph-Separators.html#igraph_all_minimal_st_separators
There was a problem hiding this comment.
Ok thanks for the clarification! Missed the part that they are minimal for some vertex pair. I got it now!
Co-authored-by: Maëlle Salmon <[email protected]>
Co-authored-by: Maëlle Salmon <[email protected]>
All tests related to functions in the file
flow.Rwere moved totest-flow.Rand refactored to fit https://masalmon.eu/2024/05/23/refactoring-tests/.Some additional tests were added and code cleaned in
flow.Rcc @maelle keeping this on draft, would like to get your opinion on the structure 😁