refactor: forward get.adjacency.dense() to get.adjacency.sparse() if attributes are present (#1483)

R/conversion.R

@@ -1,4 +1,3 @@
-
 #' Convert igraph graphs to graphNEL objects from the graph package
 #'
 #' @description
@@ -159,11 +158,6 @@ get.adjacency.dense <- function(graph, type = c("both", "upper", "lower"),
   ensure_igraph(graph)
 
   type <- igraph.match.arg(type)
-  type <- switch(type,
-    "upper" = 0,
-    "lower" = 1,
-    "both" = 2
-  )
 
   if (is.logical(loops)) {
     loops <- ifelse(loops, "once", "ignore")
@@ -183,61 +177,23 @@ get.adjacency.dense <- function(graph, type = c("both", "upper", "lower"),
 
   if (is.null(attr)) {
     on.exit(.Call(R_igraph_finalizer))
+    type <- switch(type,
+      "upper" = 0,
+      "lower" = 1,
+      "both" = 2
+    )
     res <- .Call(
       R_igraph_get_adjacency, graph, as.numeric(type), weights,
       loops
     )
   } else {
-    attr <- as.character(attr)
-    if (!attr %in% edge_attr_names(graph)) {
-      stop("no such edge attribute")
-    }
-    exattr <- edge_attr(graph, attr)
-    if (is.logical(exattr)) {
-      res <- matrix(FALSE, nrow = vcount(graph), ncol = vcount(graph))
-    } else if (is.numeric(exattr)) {
-      res <- matrix(0, nrow = vcount(graph), ncol = vcount(graph))
-    } else {
-      stop(
-        "Matrices must be either numeric or logical, ",
-        "and the edge attribute is not"
-      )
-    }
-    if (is_directed(graph)) {
-      for (i in seq(length.out = ecount(graph))) {
-        e <- ends(graph, i, names = FALSE)
-        res[e[1], e[2]] <- exattr[i]
-      }
-    } else {
-      if (type == 0) {
-        ## upper
-        for (i in seq(length.out = ecount(graph))) {
-          e <- ends(graph, i, names = FALSE)
-          res[min(e), max(e)] <- exattr[i]
-        }
-      } else if (type == 1) {
-        ## lower
-        for (i in seq(length.out = ecount(graph))) {
-          e <- ends(graph, i, names = FALSE)
-          res[max(e), min(e)] <- exattr[i]
-        }
-      } else if (type == 2) {
-        ## both
-        for (i in seq(length.out = ecount(graph))) {
-          e <- ends(graph, i, names = FALSE)
-          res[e[1], e[2]] <- exattr[i]
-          if (e[1] != e[2]) {
-            res[e[2], e[1]] <- exattr[i]
-          }
-        }
-      }
-    }
+    # faster than a specialized implementation
+    res <- as.matrix(get.adjacency.sparse(graph, type = type, attr = attr, names = names))
   }
 
   if (names && "name" %in% vertex_attr_names(graph)) {
     colnames(res) <- rownames(res) <- V(graph)$name
   }
-
   res
 }
 
@@ -1004,7 +960,7 @@ get.incidence.sparse <- function(graph, types, names, attr) {
 #' as_biadjacency_matrix(g)
 #'
 as_biadjacency_matrix <- function(graph, types = NULL, attr = NULL,
-                                names = TRUE, sparse = FALSE) {
+                                  names = TRUE, sparse = FALSE) {
   # Argument checks
   ensure_igraph(graph)
   types <- handle_vertex_type_arg(types, graph)
@@ -1033,8 +989,8 @@ as_biadjacency_matrix <- function(graph, types = NULL, attr = NULL,
 #' this naming to avoid confusion with the edge-vertex incidence matrix.
 #' @export
 as_incidence_matrix <- function(...) { # nocov start
-   lifecycle::deprecate_soft("1.6.0", "as_incidence_matrix()", "as_biadjacency_matrix()")
-   as_biadjacency_matrix(...)
+  lifecycle::deprecate_soft("1.6.0", "as_incidence_matrix()", "as_biadjacency_matrix()")
+  as_biadjacency_matrix(...)
 } # nocov end
 #' @rdname graph_from_data_frame
 #' @param x An igraph object.

tests/testthat/_snaps/conversion.md

@@ -41,14 +41,14 @@
     Code
       as_adjacency_matrix(g, attr = "bla", sparse = FALSE)
     Condition
-      Error in `get.adjacency.dense()`:
+      Error in `get.adjacency.sparse()`:
       ! no such edge attribute
 
 ---
 
     Code
       as_adjacency_matrix(g, attr = "bla", sparse = FALSE)
     Condition
-      Error in `get.adjacency.dense()`:
+      Error in `get.adjacency.sparse()`:
       ! Matrices must be either numeric or logical, and the edge attribute is not
 

tests/testthat/test-conversion.R

@@ -71,7 +71,7 @@ test_that("as_undirected() keeps attributes", {
 })
 
 test_that("as_adjacency_matrix() works -- sparse", {
-  g <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed = TRUE)
+  g <- make_graph(c(1, 2, 2, 1, 2, 2, 3, 3, 3, 3, 3, 4, 4, 2, 4, 2, 4, 2), directed = TRUE)
   basic_adj_matrix <- as_adjacency_matrix(g)
   expect_s4_class(basic_adj_matrix, "dgCMatrix")
   expected_matrix <- matrix(
@@ -93,17 +93,19 @@ test_that("as_adjacency_matrix() works -- sparse", {
   E(g)$weight <- c(1.2, 3.4, 2.7, 5.6, 6.0, 0.1, 6.1, 3.3, 4.3)
   weight_adj_matrix <- as_adjacency_matrix(g, attr = "weight")
   expect_s4_class(weight_adj_matrix, "dgCMatrix")
-  expect_equal(as.matrix(weight_adj_matrix),
+  expect_equal(
+    as.matrix(weight_adj_matrix),
     matrix(
       c(0, 3.4, 0, 0, 1.2, 2.7, 0, 13.7, 0, 0, 11.6, 0, 0, 0, 0.1, 0),
       nrow = 4L,
       ncol = 4L,
       dimnames = list(c("a", "b", "c", "d"), c("a", "b", "c", "d"))
-    ))
+    )
+  )
 })
 
 test_that("as_adjacency_matrix() works -- sparse + not both", {
-  dg <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed = TRUE)
+  dg <- make_graph(c(1, 2, 2, 1, 2, 2, 3, 3, 3, 3, 3, 4, 4, 2, 4, 2, 4, 2), directed = TRUE)
   g <- as_undirected(dg, mode = "each")
 
   lower_adj_matrix <- as_adjacency_matrix(g, type = "lower")
@@ -128,16 +130,15 @@ test_that("as_adjacency_matrix() works -- sparse + not both", {
 })
 
 test_that("as_adjacency_matrix() errors well -- sparse", {
-  g <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed = TRUE)
+  g <- make_graph(c(1, 2, 2, 1, 2, 2, 3, 3, 3, 3, 3, 4, 4, 2, 4, 2, 4, 2), directed = TRUE)
   expect_snapshot(as_adjacency_matrix(g, attr = "bla"), error = TRUE)
 
   E(g)$bla <- letters[1:ecount(g)]
   expect_snapshot(as_adjacency_matrix(g, attr = "bla"), error = TRUE)
-
 })
 
 test_that("as_adjacency_matrix() works -- sparse undirected", {
-  dg <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed = TRUE)
+  dg <- make_graph(c(1, 2, 2, 1, 2, 2, 3, 3, 3, 3, 3, 4, 4, 2, 4, 2, 4, 2), directed = TRUE)
   ug <- as_undirected(dg, mode = "each")
   adj_matrix <- as_adjacency_matrix(ug)
   expect_s4_class(adj_matrix, "dgCMatrix")
@@ -155,7 +156,7 @@ test_that("as_adjacency_matrix() works -- sparse undirected", {
 })
 
 test_that("as_adjacency_matrix() works -- dense", {
-  g <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed = TRUE)
+  g <- make_graph(c(1, 2, 2, 1, 2, 2, 3, 3, 3, 3, 3, 4, 4, 2, 4, 2, 4, 2), directed = TRUE)
 
   basic_adj_matrix <- as_adjacency_matrix(g, sparse = FALSE)
   expected_matrix <- matrix(
@@ -175,7 +176,11 @@ test_that("as_adjacency_matrix() works -- dense", {
   expect_equal(
     weight_adj_matrix,
     matrix(
-      c(0, 3.4, 0, 0, 1.2, 2.7, 0, 4.3, 0, 0, 6, 0, 0, 0, 0.1, 0),
+      c(0, 3.4, 0, 0, 1.2, 2.7, 0, 13.7, 0, 0, 11.6, 0, 0, 0, 0.1, 0),
+      # below is wrong test result due to a bug (#1551). Weights of ties
+      # between the same node pair should be aggregated and not only the last
+      # weight should be considered. The above is consistent with the sparse case
+      # c(0, 3.4, 0, 0, 1.2, 2.7, 0, 4.3, 0, 0, 6, 0, 0, 0, 0.1, 0),
       nrow = 4L,
       ncol = 4L,
       dimnames = list(c("a", "b", "c", "d"), c("a", "b", "c", "d"))
@@ -184,17 +189,16 @@ test_that("as_adjacency_matrix() works -- dense", {
 })
 
 test_that("as_adjacency_matrix() errors well -- dense", {
-  g <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed = TRUE)
+  g <- make_graph(c(1, 2, 2, 1, 2, 2, 3, 3, 3, 3, 3, 4, 4, 2, 4, 2, 4, 2), directed = TRUE)
   expect_snapshot(as_adjacency_matrix(g, attr = "bla", sparse = FALSE), error = TRUE)
 
   E(g)$bla <- letters[1:ecount(g)]
   expect_snapshot(as_adjacency_matrix(g, attr = "bla", sparse = FALSE), error = TRUE)
-
 })
 
 
 test_that("as_adjacency_matrix() works -- dense undirected", {
-  dg <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed = TRUE)
+  dg <- make_graph(c(1, 2, 2, 1, 2, 2, 3, 3, 3, 3, 3, 4, 4, 2, 4, 2, 4, 2), directed = TRUE)
   ug <- as_undirected(dg, mode = "each")
   # no different treatment than undirected if no attribute?!
   adj_matrix <- as_adjacency_matrix(ug, sparse = FALSE)
@@ -211,15 +215,19 @@ test_that("as_adjacency_matrix() works -- dense undirected", {
   expect_equal(
     weight_adj_matrix,
     matrix(
-      c(0, 3.4, 0, 0, 3.4, 2.7, 0, 4.3, 0, 0, 6, 0.1, 0, 4.3, 0.1, 0),
+      c(0, 4.6, 0, 0, 4.6, 2.7, 0, 13.7, 0, 0, 11.6, 0.1, 0, 13.7, 0.1, 0),
+      # below is wrong test result due to a bug (#1551). Weights of ties
+      # between the same node pair should be aggregated and not only the last
+      # weight should be considered. The above is consistent with the sparse case
+      # c(0, 3.4, 0, 0, 3.4, 2.7, 0, 4.3, 0, 0, 6, 0.1, 0, 4.3, 0.1, 0),
       nrow = 4L,
       ncol = 4L
     )
   )
 })
 
 test_that("as_adjacency_matrix() works -- dense + not both", {
-  dg <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed = TRUE)
+  dg <- make_graph(c(1, 2, 2, 1, 2, 2, 3, 3, 3, 3, 3, 4, 4, 2, 4, 2, 4, 2), directed = TRUE)
   g <- as_undirected(dg, mode = "each")
   E(g)$attribute <- c(1.2, 3.4, 2.7, 5.6, 6.0, 0.1, 6.1, 3.3, 4.3)
 
@@ -233,13 +241,17 @@ test_that("as_adjacency_matrix() works -- dense + not both", {
   expect_equal(
     lower_adj_matrix,
     matrix(
-      c(0, 3.4, 0, 0, 0, 2.7, 0, 4.3, 0, 0, 6, 0.1, 0, 0, 0, 0),
+      c(0, 4.6, 0, 0, 0, 2.7, 0, 13.7, 0, 0, 11.6, 0.1, 0, 0, 0, 0),
+      # below is wrong test result due to a bug (#1551). Weights of ties
+      # between the same node pair should be aggregated and not only the last
+      # weight should be considered. The above is consistent with the sparse case
+      # c(0, 3.4, 0, 0, 0, 2.7, 0, 4.3, 0, 0, 6, 0.1, 0, 0, 0, 0),
       nrow = 4L,
       ncol = 4L
     )
   )
 
-  upper_adj_matrix  <- as_adjacency_matrix(
+  upper_adj_matrix <- as_adjacency_matrix(
     g,
     type = "upper",
     sparse = FALSE,
@@ -249,7 +261,11 @@ test_that("as_adjacency_matrix() works -- dense + not both", {
   expect_equal(
     upper_adj_matrix,
     matrix(
-      c(0, 0, 0, 0, 3.4, 2.7, 0, 0, 0, 0, 6, 0, 0, 4.3, 0.1, 0),
+      c(0, 0, 0, 0, 4.6, 2.7, 0, 0, 0, 0, 11.6, 0, 0, 13.7, 0.1, 0),
+      # below is wrong test result due to a bug (#1551). Weights of ties
+      # between the same node pair should be aggregated and not only the last
+      # weight should be considered. The above is consistent with the sparse case
+      # c(0, 0, 0, 0, 3.4, 2.7, 0, 0, 0, 0, 6, 0, 0, 4.3, 0.1, 0),
       nrow = 4L,
       ncol = 4L
     )