Bioconductor / graph

graph: A package to handle graph data structures
https://bioconductor.org/packages/graph
4 stars 9 forks source link

Bug in edgeMatrix #1

Closed jawitte closed 3 years ago

jawitte commented 3 years ago

Hello, I think I found a bug in the edgeMatrix function. When the input is a graphAM object with an adjacency matrix that has exactly one entry in each row, then 'to' is a vector instead of a list and the next line cannot be executed. Here is a minimal example with a (2x2)-matrix:

library(graph) # version 1.66.0 adjm <- matrix(c(0,1,1,0), nrow=2) # this matrix has exactly one entry in each row obj <- graphAM(adjMat=adjm, edgemode="directed") edgeMatrix(obj) # Error

from the source code of edgeMatrix (I replaced 'object' with 'obj'):

to <- apply(obj@adjMat, 1, function(x) which(x != 0)) # 'to' is a vector from <- rep(seq_len(numNodes(obj)), graph:::listLen(to)) # Error

If the adjacency matrix has rows without entries, or rows with more than one entry, then the error does not occur, as in this unproblematic example: adjm_2 <- matrix(c(0,1,1, 1,0,0, 1,0,0), nrow=3) obj_2 <- graphAM(adjMat=adjm_2, edgemode="directed") edgeMatrix(obj_2)

from the source code of edgeMatrix (I replaced 'object' with 'obj_2'):

to <- apply(obj_2@adjMat, 1, function(x) which(x != 0)) # 'to' is a list from <- rep(seq_len(numNodes(obj_2)), graph:::listLen(to))

vjcitn commented 3 years ago

Hi, thanks for the detailed report. I think I have straightened this out in a personal clone of graph, which I would like you to try on your real problem. Use BiocManager::install("vjcitn/graph") to get the fixed version. If it pans out I will commit the changes to master. Specifically, the edgeMatrix function was modified to ensure listLen gets a suitable input, and listLen got a little bulletproofing.

> to <- apply(obj@adjMat, 1, function(x) which(x != 0)) # 'to' is a vector

> from <- try(rep(seq_len(numNodes(obj)), graph:::listLen(to))) # Error
Error in graph:::listLen(to) : is(list, "list") is not TRUE
jawitte commented 3 years ago

Dear Vince, thank you for your answer! With the new version of the package, I still get an error message, but a different one. I wrote a new minimal example (see below). The problem no longer occurs if each row in the adjacency matrix has exactly one entry, but it still occurs if each row has exactly m>1 entries. The apply function then chooses to output a matrix. The new code "to <- lapply(to, force)" transforms the matrix into a list, but the list has too many elements because it considers each element of the matrix separately.

library(graph) # version 1.71.1

adjm <- matrix(c(0,1,1, 1,0,1, 0,1,1), nrow=3, byrow=TRUE) # this matrix has exactly two entries in each row obj <- graphAM(adjMat=adjm, edgemode="directed") edgeMatrix(obj) # Error

from the source code of edgeMatrix (I replaced 'object' with 'obj'):

to <- apply(obj@adjMat, 1, function(x) which(x != 0)) # 'to' is a matrix to <- lapply(to, force) # fixup to allow listLen to work from <- rep(seq_len(numNodes(obj)), graph:::listLen(to)) # Error

vjcitn commented 3 years ago

Thank you for your patience. vjcitn/graph now uses simplify=FALSE in the computation of "to" which seems more appropriate for producing a list for all inputs. please give it a try. unit tests were added for your examples.

jawitte commented 3 years ago

Oh, since when does 'apply' have a 'simplify=TRUE' option? That's great news actually, I wasn't aware of that option! I just updated my R version, now everything runs smoothly, all error messages are gone. Thank you!