feat: added simple check to graph_from_literal()
#1981
Conversation
schochastics
changed the title
graph_from_literal()
|
Too risky, let's wait. |
| ids <- seq(along.with = v) | ||
| names(ids) <- v | ||
| res <- make_graph(unname(ids[edges]), n = length(v), directed = directed) | ||
| if (simplify) { |
There was a problem hiding this comment.
why couldn't the check live inside simplify()?
There was a problem hiding this comment.
i.e., could simplify() be idempotent? https://en.wikipedia.org/wiki/Idempotence
There was a problem hiding this comment.
@maelle It is not idempotent because checking if a graph is simple takes time. Simplify will perform the simplification procedure either way, and this may reorder the edges of the graph, even if the graph is simple. The solution @schochastics uses here, i.e. check whether the graph is simple first, is good, and should be kept. The point is not to mess up the edge order for no good reason, and make it easier for people to add edge attributes when constructing from a literal.
It's good to note again that igraph has a cache for basic graph properties, such as whether the graph is simple. If it was cached that the graph is simple, then: is_simple() takes O(1) times, and simplify() does nothing.
I hope this also answers why the check should not be in simplify():
- Because it takes time and would slow
simplify()down - There is already a kind of check in
simplify(), but not for whether the graph is simple, but for whether the graph is already known to be simple (i.e. whether this is cached).
There was a problem hiding this comment.
thank you!! Yes, it makes sense.
4 participants
Fix #824
The PR implements the suggested additional check if a graph is simple or not.
This required a lot of adjustments in the tests given that this means that the edge order was changed. I could imagine that this might lead to issues downstream so keeping this for after the 2.2 release.
cc @szhorvat is this what you intended?