Skip to content

feat: added simple check to graph_from_literal() #1981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+38 −13
Draft

feat: added simple check to graph_from_literal() #1981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b06d58
added simple check to graph_from_literal
schochastics Jul 15, 2025
aa0695a
adjusted tests to new graph_from_literal
schochastics Jul 15, 2025
4ac7fde
Merge branch 'main' into simplify-literal
schochastics Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R/make.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1819,7 +1819,7 @@ graph_from_literal_i <- function(mf) {
ids <- seq(along.with = v)
names(ids) <- v
res <- make_graph(unname(ids[edges]), n = length(v), directed = directed)
if (simplify) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why couldn't the check live inside simplify()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e., could simplify() be idempotent? https://en.wikipedia.org/wiki/Idempotence

Copy link
Member

@szhorvat szhorvat Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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():

  1. Because it takes time and would slow simplify() down
  2. 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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!! Yes, it makes sense.

if (simplify && !is_simple(res)) {
res <- simplify(res)
}
res <- set_vertex_attr(res, "name", value = v)
Expand Down
6 changes: 3 additions & 3 deletions tests/testthat/_snaps/make.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
IGRAPH UN-- 3 3 --
+ attr: name (v/c)
+ edges (vertex names):
[1] A--B A--C B--C
[1] A--B B--C A--C

# graph_from_literal() and undirected explosion

Expand Down Expand Up @@ -112,7 +112,7 @@
IGRAPH DN-- 3 3 --
+ attr: name (v/c)
+ edges (vertex names):
[1] A->B C->A C->B
[1] A->B C->B C->A

# graph_from_literal() and directed explosion

Expand All @@ -129,7 +129,7 @@
IGRAPH DN-- 13 19 --
+ attr: name (v/c)
+ edges (vertex names):
[1] A->D A->E B->D B->E C->D C->E F->D F->E F->I G->D G->E G->I H->D H->E H->I
[1] A->D A->E B->D B->E C->D C->E F->D G->D H->D F->E G->E H->E F->I G->I H->I
[16] J->I K->I L->I M->I

# graph_from_literal(simplify = FALSE)
Expand Down
3 changes: 2 additions & 1 deletion tests/testthat/_snaps/structural-properties.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
# bfs() works

Code
g <- graph_from_literal(a - +b - +c, z - +a, d)
el <- structure(c("a", "b", "z", "b", "c", "a"), dim = 3:2)
g <- graph_from_edgelist(el) + vertex("d")
bfs(g, root = 2, mode = "out", unreachable = FALSE, order = TRUE, rank = TRUE,
parent = TRUE, pred = TRUE, succ = TRUE, dist = TRUE)
Output
Expand Down
9 changes: 7 additions & 2 deletions tests/testthat/test-conversion.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ test_that("as_directed works", {
})

test_that("as_directed keeps attributes", {
g <- graph_from_literal(A - B - C, D - A, E)
el <- structure(c("A", "A", "B", "B", "D", "C"), dim = 3:2)
g <- graph_from_edgelist(el, directed = FALSE) + vertex("E")
g$name <- "Small graph"
g_mutual <- as_directed(g, mode = "mutual")
expect_equal(g_mutual$name, g$name)
Expand Down Expand Up @@ -59,7 +60,11 @@ test_that("as.undirected() deprecation", {
})

test_that("as_undirected() keeps attributes", {
g <- graph_from_literal(A +-+ B, A --+ C, C +-+ D)
el <- structure(
c("A", "A", "B", "C", "D", "B", "C", "A", "D", "C"),
dim = c(5L, 2L)
)
g <- graph_from_edgelist(el, directed = TRUE)
g$name <- "Tiny graph"
E(g)$weight <- seq_len(ecount(g))

Expand Down
17 changes: 14 additions & 3 deletions tests/testthat/test-flow.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,13 @@ test_that("st_cuts() works", {
expect_equal(unvs(all_cuts_path$cuts), list(1, 2, 3, 4))
expect_equal(unvs(all_cuts_path$partition1s), list(1, 1:2, 1:3, 1:4))

g_star_v7 <- graph_from_literal(s -+ a:b -+ t, a -+ 1:2:3 -+ b)
el <- structure(
c(
"s", "s", "a", "a", "a", "a", "b", "1", "2", "3",
"a", "b", "t", "1", "2", "3", "t", "b", "b", "b"),
dim = c(10L, 2L)
)
g_star_v7 <- graph_from_edgelist(el)
all_cuts_star_v7 <- st_cuts(g_star_v7, source = "s", target = "t")
expect_equal(
unvs(all_cuts_star_v7$cuts),
Expand Down Expand Up @@ -84,8 +90,13 @@ test_that("st_cuts() works", {
c(1, 2, 5, 6, 7, 3)
)
)

g_star_v9 <- graph_from_literal(s -+ a:b -+ t, a -+ 1:2:3:4:5 -+ b)
el <- structure(
c("s", "s", "a", "a", "a", "a", "a", "a", "b", "1",
"2", "3", "4", "5", "a", "b", "t", "1", "2", "3", "4", "5", "t",
"b", "b", "b", "b", "b"),
dim = c(14L, 2L)
)
g_star_v9 <- graph_from_edgelist(el)
all_cuts_star_v9 <- st_min_cuts(g_star_v9, source = "s", target = "t")
expect_equal(all_cuts_star_v9$value, 2)
expect_equal(unvs(all_cuts_star_v9$cuts), list(c(1, 2), c(1, 9), c(3, 9)))
Expand Down
11 changes: 9 additions & 2 deletions tests/testthat/test-operators.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,15 @@ test_that("compose() works", {
})

test_that("compose works for named graphs", {
g1 <- graph_from_literal(A - B:D:E, B - C:D, C - D, D - E)
g2 <- graph_from_literal(A - B - E - A)
el1 <- structure(
c(
"A", "A", "A", "B", "B", "D", "D", "B", "D", "E",
"D", "C", "E", "C"),
dim = c(7L, 2L)
)
el2 <- structure(c("A", "A", "B", "B", "E", "E"), dim = 3:2)
g1 <- graph_from_edgelist(el1, directed = FALSE)
g2 <- graph_from_edgelist(el2, directed = FALSE)

V(g1)$bar1 <- seq_len(vcount(g1))
V(g2)$bar2 <- seq_len(vcount(g2))
Expand Down
3 changes: 2 additions & 1 deletion tests/testthat/test-structural-properties.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ test_that("bfs() works", {
local_igraph_options(print.id = FALSE)

expect_snapshot({
g <- graph_from_literal(a -+ b -+ c, z -+ a, d)
el <- structure(c("a", "b", "z", "b", "c", "a"), dim = 3:2)
g <- graph_from_edgelist(el) + vertex("d")
bfs(
g,
root = 2,
Expand Down
Loading