Skip to content
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.

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

inconsistent behaviour of get.adjacency.dense and get.adjacency.sparse for weighted multigraphs #1551

Open
schochastics opened this issue Oct 10, 2024 · 4 comments

Comments

@schochastics
Copy link

What happens, and what did you expect instead?

The internal functions get.adjacency.dense() and get.adjacency.sparse() return different results for weighted multigraphs. get.adjacency.sparse() aggregates weights for multi edges and get.adjacency.dense() reports the weight of the last edge. In my opinion, the behavior of get.adjacency.sparse() is correct. The PR #1518 coincidentally fixes this.

Given this is indeed a bug, then the following test should be fixed either in a new PR or the existing PR #1518 (see also the discussion there)

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),
nrow = 4L,
ncol = 4L,
dimnames = list(c("a", "b", "c", "d"), c("a", "b", "c", "d"))
)
)

To reproduce

igraph 2.0.3

library(igraph)
#> 
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#> 
#>     decompose, spectrum
#> The following object is masked from 'package:base':
#> 
#>     union
g <- make_graph(c(1,2, 2,1, 2,2, 3,3, 3,3, 3,4, 4,2, 4,2, 4,2), directed = TRUE)
E(g)$weight <- c(1.2, 3.4, 2.7, 5.6, 6.0, 0.1, 6.1, 3.3, 4.3)
as_adjacency_matrix(g, attr = "weight", sparse = FALSE)
#>      [,1] [,2] [,3] [,4]
#> [1,]  0.0  1.2    0  0.0
#> [2,]  3.4  2.7    0  0.0
#> [3,]  0.0  0.0    6  0.1
#> [4,]  0.0  4.3    0  0.0
as_adjacency_matrix(g, attr = "weight", sparse = TRUE)
#> 4 x 4 sparse Matrix of class "dgCMatrix"
#>                       
#> [1,] .    1.2  .   .  
#> [2,] 3.4  2.7  .   .  
#> [3,] .    .   11.6 0.1
#> [4,] .   13.7  .   .

Created on 2024-10-10 with reprex v2.1.1

System information

No response

@szhorvat
Copy link
Member

szhorvat commented Oct 11, 2024

Yes, the weights should be added up. Thanks for catching this!

IMO the best solution here is to call the C core, which is not happening now. Relying on the C core ensures consistency and reduces code duplication.

The C core does not support Boolean adjacency matrices, so it is to be decided what to do with that. Options are to implement this in R, or to convert logical values to numeric, then convert back again. It's also a question whether logicals should be supported. One argument in favour is that it's not too difficult to do so.

@schochastics
Copy link
Author

So what's your take on PR #1518?
Does it make sense to merge optimised R code or should this be replaced entirely by calls to the C core (when possible)?

@maelle
Copy link
Contributor

maelle commented Oct 17, 2024

@szhorvat friendly reminder about @schochastics' question above. Is the conversion of sparse matrices from C to R and vice versa still a blocker for this?

@maelle
Copy link
Contributor

maelle commented Oct 17, 2024

related (I think?) #1137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants