attr in as_biadjacency_matrix()

[maelle]

@szhorvat is this expected:

library("igraph")
#> 
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#> 
#>     decompose, spectrum
#> The following object is masked from 'package:base':
#> 
#>     union

withr::local_seed(42)

I <- matrix(sample(0:1, 9, replace = TRUE, prob = c(3, 1)), ncol = 3)

g <- graph_from_biadjacency_matrix(I)
E(g)$something <- runif(ecount(g))
as_biadjacency_matrix(g, sparse = TRUE, attr = "something")
#> 3 x 3 sparse Matrix of class "dgTMatrix"
#>           4         5 6
#> 1 0.7050648 0.4577418 .
#> 2 0.7191123 .         .
#> 3 .         .         .

g2 <- graph_from_biadjacency_matrix(I)
E(g2)$something <- letters[1:ecount(g)]
as_biadjacency_matrix(g2, sparse = TRUE, attr = "something")
#> Error in getClass(Class, where = topenv(parent.frame())): "gTMatrix" is not a defined class

^{Created on 2024-09-30 with reprex v2.1.0}

If so, we should add some details to the documentation about what attrcan be depending on sparse/dense, and some argument checks inside the function.

[szhorvat]

attr should be replaced completely, as I proposed in #906. It is an unfortunate design that makes some features difficult to implement. External contributions were blocked by this (see PRs and issues linked from #906).

I am not sure if the behaviour is expected, as it is pure R code and I am not sure what gTMatrix is. Still, parts of the (bi)adjacency operations are implemented in pure R. Ideally these would be replaced by calling the C versions, which would ensure full consistency between dense/sparse operations, as well as the different interfaces. One of the blockers for this was implementing moving sparse matrices to/from R.

Google tells me that gTMatrix refers to a triplet form. Triplet forms are only useful for building sparse matrices, not for computing with them. See this answer from an expert on the topic: DrTimothyAldenDavis/SuiteSparse#763

[maelle]

Oh right I remember that now, thank you. I would say that with #1518 and my having now looked a bit more at the code we're closer to actually doing that.

Ok not to add tests for this case for now I'd say (which is why I had opened the issue).