neighbors bug

[gaborcsardi]

library(igraph)
graph <- make_ring(10)
neighbors(graph, numeric())
#> + 2/10 vertices:
#> [1]  2 10

[kdaily]

I see this as well in version 1.0.1 - if the list of vertices supplied is empty, there are still edges returned.

[szhorvat]

Isn't this a bug in how arguments are translated to C? The v argument of neighbors should be precisely one integer, not zero, not more than one. The argument translator should catch cases when this is not true. The same thing causes trouble in #140.

[ntamas]

It is most likely a bug in the internal as.igraph.vs() function at the end of iterators.R. The relevant part of the code looks like this:

if (is.logical(v)) {
  res <- as.vector(V(graph))[v]
} else if (is.numeric(v) && any(v<0)){
  res <- as.vector(V(graph))[v]
} else {
  res <- as.numeric(v)
}

numeric() fails the first two branches so it is treated as res <- as.numeric(numeric()), yielding numeric(0), i.e. a zero-length vector. The code that calls as.igraph.vs() then takes the first element of the vector and decreases it by 1.

I guess what we would need is an additional check like this:

if (length(res) == 0) {
  stop("No vertex name was given")
}

A similar error exists in as.igraph.es as well.

[szhorvat]

Should this even use as.igraph.vs? This function takes a single vertex ID, not a vertex selector.

https://igraph.org/c/doc/igraph-Basic.html#igraph_neighbors

rigraph/tools/stimulus/rinterface.c.in

Line 3687 in a14fc50

SEXP R_igraph_neighbors(SEXP graph, SEXP pvid, SEXP pmode) {

I would imagine (but I don't know the details) that the line vid=REAL(pvid)[0]; interprets the input as a vector of reals and takes the first element. But numeric(0) is a zero-length vector, so I assume this accesses memory it shouldn't.

It's similar in #140

[ntamas]

The R_igraph_neighbors function was written under the assumption that it receives a REALSXP (i.e. a numeric vector), and the neighbors() function in R is also totally happy with a vector of length 1 as the input. That's why the neighbors() function in R calls as.igraph.vs(). If we don't want to break the API, we need to deal with vectors somewhere, either in the R layer or in the C layer. I vote for R as it's always easier to work on a higher level.

This means that maybe we should have a variant of as.igraph.vs() that simply calls as.igraph.vs(), checks the length of the result vector and stops if it is not 1. We can then use this function in sensitive places like neighbors() or incident(), where the underlying C function needs a single ID.

[ntamas]

So, after giving it a second thought, I'd say that as.igraph.vs() and as.igraph.es() should be left as is; they work correctly when given numeric() as an input -- they get an empty numeric vector and return an empty numeric vector, so that's all right. What is needed, though, is that functions that need exactly one vertex or edge ID should check the length of whatever is returned from as.igraph.vs() and as.igraph.es() and print an error if needed.

[ntamas]

I've prevented functions taking a single vertex ID from accepting an empty vector so this should be fixed now.