From c1373e5f26dc13738174dfed92145e78badcedd0 Mon Sep 17 00:00:00 2001 From: schochastics Date: Wed, 16 Apr 2025 20:50:30 +0200 Subject: [PATCH 1/6] added NA handling for matrix inputs --- R/adjacency.R | 1 + R/conversion.R | 9 ++++----- R/incidence.R | 1 + R/utils-assert-args.R | 6 ++++++ tests/testthat/_snaps/adjacency.md | 8 ++++++++ tests/testthat/_snaps/conversion.md | 8 ++++++++ tests/testthat/_snaps/incidence.md | 8 ++++++++ tests/testthat/test-adjacency.R | 9 ++++++--- tests/testthat/test-conversion.R | 7 ++++++- tests/testthat/test-incidence.R | 5 +++++ 10 files changed, 53 insertions(+), 9 deletions(-) diff --git a/R/adjacency.R b/R/adjacency.R index 9ebf52700c..c32240ae6e 100644 --- a/R/adjacency.R +++ b/R/adjacency.R @@ -228,6 +228,7 @@ graph_from_adjacency_matrix <- function(adjmatrix, ), weighted = NULL, diag = TRUE, add.colnames = NULL, add.rownames = NA) { + ensure_no_na(adjmatrix, "adjacency matrix", call = rlang::caller_env()) mode <- igraph.match.arg(mode) if (!is.matrix(adjmatrix) && !inherits(adjmatrix, "Matrix")) { diff --git a/R/conversion.R b/R/conversion.R index d378893e1f..f466cb5bb2 100644 --- a/R/conversion.R +++ b/R/conversion.R @@ -1397,10 +1397,8 @@ graph_from_data_frame <- function(d, directed = TRUE, vertices = NULL) { } ## Handle if some elements are 'NA' - if (any(is.na(d[, 1:2]))) { - cli::cli_warn("In {.code d}, {.code NA} elements were replaced with string {.str NA}.") - d[, 1:2][is.na(d[, 1:2])] <- "NA" - } + ensure_no_na(d, "edge data frame", call = rlang::caller_env()) + if (!is.null(vertices) && any(is.na(vertices[, 1]))) { cli::cli_warn("In {.code vertices[,1]}, {.code NA} elements were replaced with string {.str NA}.") vertices[, 1][is.na(vertices[, 1])] <- "NA" @@ -1495,8 +1493,9 @@ from_data_frame <- function(...) constructor_spec(graph_from_data_frame, ...) #' graph_from_edgelist(cbind(1:10, c(2:10, 1))) graph_from_edgelist <- function(el, directed = TRUE) { if (!is.matrix(el) || ncol(el) != 2) { - cli::cli_abort("graph_from_edgelist expects a matrix with two columns") + cli::cli_abort("graph_from_edgelist expects a matrix with two columns.") } + ensure_no_na(el, "edgelist", call = rlang::caller_env()) if (nrow(el) == 0) { res <- make_empty_graph(directed = directed) diff --git a/R/incidence.R b/R/incidence.R index dd1118b735..cefec10a60 100644 --- a/R/incidence.R +++ b/R/incidence.R @@ -168,6 +168,7 @@ graph_from_biadjacency_matrix <- function(incidence, directed = FALSE, multiple = FALSE, weighted = NULL, add.names = NULL) { # Argument checks + ensure_no_na(incidence, "biadjacency matrix", call = rlang::caller_env()) directed <- as.logical(directed) mode <- igraph.match.arg(mode) diff --git a/R/utils-assert-args.R b/R/utils-assert-args.R index 3bb7d16736..53f3c6e24c 100644 --- a/R/utils-assert-args.R +++ b/R/utils-assert-args.R @@ -32,3 +32,9 @@ igraph.match.arg <- function(arg, values, error_call = rlang::caller_env()) { error_call = error_call ) } + +ensure_no_na <- function(x, what, call) { + if (any(is.na(x))) { + cli::cli_abort("Cannot create a graph object because the {what} contains NAs.", call = call) + } +} diff --git a/tests/testthat/_snaps/adjacency.md b/tests/testthat/_snaps/adjacency.md index c46c414628..6a5b41d0df 100644 --- a/tests/testthat/_snaps/adjacency.md +++ b/tests/testthat/_snaps/adjacency.md @@ -139,3 +139,11 @@ IGRAPH U--- 2 0 -- + edges: +# graph_from_adjacency_matrix errors for NAs + + Code + graph_from_adjacency_matrix(A) + Condition + Error: + ! Cannot create a graph object because the adjacency matrix contains NAs. + diff --git a/tests/testthat/_snaps/conversion.md b/tests/testthat/_snaps/conversion.md index 8da10b7890..33624b7d48 100644 --- a/tests/testthat/_snaps/conversion.md +++ b/tests/testthat/_snaps/conversion.md @@ -87,3 +87,11 @@ 2 2 3 2 b B c C 3 1 3 1 a A c C +# graph_from_edgelist errors for NAs + + Code + graph_from_edgelist(A) + Condition + Error: + ! Cannot create a graph object because the edgelist contains NAs. + diff --git a/tests/testthat/_snaps/incidence.md b/tests/testthat/_snaps/incidence.md index 2b9e6ff16a..81283445e3 100644 --- a/tests/testthat/_snaps/incidence.md +++ b/tests/testthat/_snaps/incidence.md @@ -85,3 +85,11 @@ ! `multiple` and `weighted` cannot be both `TRUE`. igraph either interprets numbers larger than 1 as weights or as multiplicities, but it cannot be both. +# graph_from_biadjacency_matrix errors for NAs + + Code + graph_from_biadjacency_matrix(A) + Condition + Error: + ! Cannot create a graph object because the biadjacency matrix contains NAs. + diff --git a/tests/testthat/test-adjacency.R b/tests/testthat/test-adjacency.R index 2e9983b99f..0812b24973 100644 --- a/tests/testthat/test-adjacency.R +++ b/tests/testthat/test-adjacency.R @@ -670,7 +670,6 @@ test_that("sparse/dense matrices no loops works", { g <- graph_from_adjacency_matrix(A, diag = FALSE) expect_ecount(g, 1) expect_equal(get_edge_ids(g, c(1, 2)), 1) - }) test_that("sparse/dense matrices multiple works", { @@ -681,11 +680,10 @@ test_that("sparse/dense matrices multiple works", { expect_ecount(g, 3) expect_equal(as_edgelist(g), matrix(c(1, 2), 3, 2, byrow = TRUE)) - A <- as(A,"dgCMatrix") + A <- as(A, "dgCMatrix") g <- graph_from_adjacency_matrix(A, diag = FALSE) expect_ecount(g, 3) expect_equal(as_edgelist(g), matrix(c(1, 2), 3, 2, byrow = TRUE)) - }) test_that("sparse/dense matrices min/max/plus", { @@ -712,3 +710,8 @@ test_that("sparse/dense matrices min/max/plus", { g <- graph_from_adjacency_matrix(A, diag = FALSE, mode = "plus", weighted = TRUE) expect_equal(E(g)$weight[1], 5) }) + +test_that("graph_from_adjacency_matrix errors for NAs", { + A <- matrix(c(1, 1, NA, 1), 2, 2) + expect_snapshot(graph_from_adjacency_matrix(A), error = TRUE) +}) diff --git a/tests/testthat/test-conversion.R b/tests/testthat/test-conversion.R index 4b6a881f21..c794115d8a 100644 --- a/tests/testthat/test-conversion.R +++ b/tests/testthat/test-conversion.R @@ -53,7 +53,7 @@ test_that("as.undirected() deprecation", { }) test_that("as_undirected() keeps attributes", { - g <- graph_from_literal(A +-+ B, A --+ C, C +-+ D) + g <- graph_from_literal(A + -+B, A - -+C, C + -+D) g$name <- "Tiny graph" E(g)$weight <- seq_len(ecount(g)) @@ -596,3 +596,8 @@ test_that("edge names work", { structure(c("b", "c", "d", "e", "g", "h", "a", "c", "d", "e", "f", "h", "i", "j"), .Dim = c(7L, 2L)) ) }) + +test_that("graph_from_edgelist errors for NAs", { + A <- matrix(c(1, 2, NA, 1), 2, 2) + expect_snapshot(graph_from_edgelist(A), error = TRUE) +}) diff --git a/tests/testthat/test-incidence.R b/tests/testthat/test-incidence.R index 47034a9deb..9966a8986c 100644 --- a/tests/testthat/test-incidence.R +++ b/tests/testthat/test-incidence.R @@ -175,3 +175,8 @@ test_that("graph_from_biadjacency_matrix() errors well", { (g <- graph_from_biadjacency_matrix(inc, multiple = TRUE, weighted = TRUE)) }) }) + +test_that("graph_from_biadjacency_matrix errors for NAs", { + A <- matrix(c(1, 1, NA, 1), 2, 2) + expect_snapshot(graph_from_biadjacency_matrix(A), error = TRUE) +}) From 22a4c6e8b0fb73abba9c96728b8a9c467e5667b1 Mon Sep 17 00:00:00 2001 From: schochastics Date: Wed, 16 Apr 2025 20:53:44 +0200 Subject: [PATCH 2/6] fixed a literal --- tests/testthat/test-conversion.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-conversion.R b/tests/testthat/test-conversion.R index c794115d8a..6cc08d982d 100644 --- a/tests/testthat/test-conversion.R +++ b/tests/testthat/test-conversion.R @@ -53,7 +53,7 @@ test_that("as.undirected() deprecation", { }) test_that("as_undirected() keeps attributes", { - g <- graph_from_literal(A + -+B, A - -+C, C + -+D) + g <- graph_from_literal(A +-+ B, A --+ C, C +-+ D) g$name <- "Tiny graph" E(g)$weight <- seq_len(ecount(g)) From 82d089ec2f4c1ca8cd5762a3bbd4e92649d9fd00 Mon Sep 17 00:00:00 2001 From: schochastics Date: Tue, 29 Apr 2025 21:19:55 +0200 Subject: [PATCH 3/6] added caller_env as default --- NAMESPACE | 1 + R/adjacency.R | 2 +- R/conversion.R | 4 ++-- R/incidence.R | 2 +- R/utils-assert-args.R | 3 ++- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 19c88795d6..f0cc95bf2c 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -937,6 +937,7 @@ importFrom(rlang,"%||%") importFrom(rlang,.data) importFrom(rlang,.env) importFrom(rlang,as_function) +importFrom(rlang,caller_env) importFrom(rlang,check_dots_empty) importFrom(rlang,check_installed) importFrom(rlang,global_env) diff --git a/R/adjacency.R b/R/adjacency.R index c32240ae6e..84ded07597 100644 --- a/R/adjacency.R +++ b/R/adjacency.R @@ -228,7 +228,7 @@ graph_from_adjacency_matrix <- function(adjmatrix, ), weighted = NULL, diag = TRUE, add.colnames = NULL, add.rownames = NA) { - ensure_no_na(adjmatrix, "adjacency matrix", call = rlang::caller_env()) + ensure_no_na(adjmatrix, "adjacency matrix") mode <- igraph.match.arg(mode) if (!is.matrix(adjmatrix) && !inherits(adjmatrix, "Matrix")) { diff --git a/R/conversion.R b/R/conversion.R index f466cb5bb2..3c5bc593be 100644 --- a/R/conversion.R +++ b/R/conversion.R @@ -1397,7 +1397,7 @@ graph_from_data_frame <- function(d, directed = TRUE, vertices = NULL) { } ## Handle if some elements are 'NA' - ensure_no_na(d, "edge data frame", call = rlang::caller_env()) + ensure_no_na(d, "edge data frame") if (!is.null(vertices) && any(is.na(vertices[, 1]))) { cli::cli_warn("In {.code vertices[,1]}, {.code NA} elements were replaced with string {.str NA}.") @@ -1495,7 +1495,7 @@ graph_from_edgelist <- function(el, directed = TRUE) { if (!is.matrix(el) || ncol(el) != 2) { cli::cli_abort("graph_from_edgelist expects a matrix with two columns.") } - ensure_no_na(el, "edgelist", call = rlang::caller_env()) + ensure_no_na(el, "edgelist") if (nrow(el) == 0) { res <- make_empty_graph(directed = directed) diff --git a/R/incidence.R b/R/incidence.R index cefec10a60..9a3c90010a 100644 --- a/R/incidence.R +++ b/R/incidence.R @@ -168,7 +168,7 @@ graph_from_biadjacency_matrix <- function(incidence, directed = FALSE, multiple = FALSE, weighted = NULL, add.names = NULL) { # Argument checks - ensure_no_na(incidence, "biadjacency matrix", call = rlang::caller_env()) + ensure_no_na(incidence, "biadjacency matrix") directed <- as.logical(directed) mode <- igraph.match.arg(mode) diff --git a/R/utils-assert-args.R b/R/utils-assert-args.R index 53f3c6e24c..e66cd3b31d 100644 --- a/R/utils-assert-args.R +++ b/R/utils-assert-args.R @@ -33,7 +33,8 @@ igraph.match.arg <- function(arg, values, error_call = rlang::caller_env()) { ) } -ensure_no_na <- function(x, what, call) { +#' @importFrom rlang caller_env +ensure_no_na <- function(x, what, call = caller_env()) { if (any(is.na(x))) { cli::cli_abort("Cannot create a graph object because the {what} contains NAs.", call = call) } From 4857e1c216556a7bd1e28ef81e07b2bbaaaca37d Mon Sep 17 00:00:00 2001 From: schochastics Date: Tue, 29 Apr 2025 21:35:42 +0200 Subject: [PATCH 4/6] updated snapshots --- tests/testthat/_snaps/adjacency.md | 2 +- tests/testthat/_snaps/incidence.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/_snaps/adjacency.md b/tests/testthat/_snaps/adjacency.md index 6a5b41d0df..1af5c9cbce 100644 --- a/tests/testthat/_snaps/adjacency.md +++ b/tests/testthat/_snaps/adjacency.md @@ -144,6 +144,6 @@ Code graph_from_adjacency_matrix(A) Condition - Error: + Error in `graph_from_adjacency_matrix()`: ! Cannot create a graph object because the adjacency matrix contains NAs. diff --git a/tests/testthat/_snaps/incidence.md b/tests/testthat/_snaps/incidence.md index 81283445e3..dbb1ba9a91 100644 --- a/tests/testthat/_snaps/incidence.md +++ b/tests/testthat/_snaps/incidence.md @@ -90,6 +90,6 @@ Code graph_from_biadjacency_matrix(A) Condition - Error: + Error in `graph_from_biadjacency_matrix()`: ! Cannot create a graph object because the biadjacency matrix contains NAs. From 1d8385aae285fa81caa1641f2e60fa2417df7f82 Mon Sep 17 00:00:00 2001 From: schochastics Date: Tue, 3 Jun 2025 09:24:14 +0200 Subject: [PATCH 5/6] fixed failing test --- tests/testthat/_snaps/conversion.md | 2 +- tests/testthat/test-conversion.R | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/testthat/_snaps/conversion.md b/tests/testthat/_snaps/conversion.md index 33624b7d48..2f981abe68 100644 --- a/tests/testthat/_snaps/conversion.md +++ b/tests/testthat/_snaps/conversion.md @@ -92,6 +92,6 @@ Code graph_from_edgelist(A) Condition - Error: + Error in `graph_from_edgelist()`: ! Cannot create a graph object because the edgelist contains NAs. diff --git a/tests/testthat/test-conversion.R b/tests/testthat/test-conversion.R index b097db125f..21ba3d302f 100644 --- a/tests/testthat/test-conversion.R +++ b/tests/testthat/test-conversion.R @@ -600,6 +600,8 @@ test_that("edge names work", { test_that("graph_from_edgelist errors for NAs", { A <- matrix(c(1, 2, NA, 1), 2, 2) expect_snapshot(graph_from_edgelist(A), error = TRUE) +}) + test_that("graph_from_data_frame works with factors", { actors <- data.frame( name = c("Alice", "Bob", "Cecil", "David", "Esmeralda"), From 6380c2f222dd069b083338c055fba11334297bbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Thu, 5 Jun 2025 11:12:29 +0200 Subject: [PATCH 6/6] Update R/utils-assert-args.R --- R/utils-assert-args.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/utils-assert-args.R b/R/utils-assert-args.R index e66cd3b31d..75952217db 100644 --- a/R/utils-assert-args.R +++ b/R/utils-assert-args.R @@ -35,7 +35,7 @@ igraph.match.arg <- function(arg, values, error_call = rlang::caller_env()) { #' @importFrom rlang caller_env ensure_no_na <- function(x, what, call = caller_env()) { - if (any(is.na(x))) { + if (anyNA(x)) { cli::cli_abort("Cannot create a graph object because the {what} contains NAs.", call = call) } }