igraph / rigraph

igraph R package
https://r.igraph.org
553 stars 200 forks source link

Implement sparse in as_adjacency_matrix #1137

Open maelle opened 9 months ago

maelle commented 9 months ago

From #1071 @krlmlr I am missing some context (maybe it'll come once I read the whole thread but I am writing down items for now)

szhorvat commented 9 months ago

Sparse adjacency matrix conversion was implemented in pure R. R/igraph should use the functions from the C core instead. First step is to have efficient bi-directional conversion routines for the sparse matrix data structures.

maelle commented 9 months ago

Thank you! Then probably not for 2.0.0

szhorvat commented 9 months ago

Something to pay attention to is that there should be no differences in behaviour after the update ... As far as I can recall, differences between the R and C implementations are:

We should move away from using an edge attribute for weights when building matrices, as this is bad and limiting design (see e.g. #910 and related) We should use a separate weight parameter, as is now done in the C core, #906.

krlmlr commented 9 months ago

From #1071.

maelle commented 1 month ago

I stumbled upon this when adding tests and wondering why the loops argument wasn't exposed to users.

szhorvat commented 1 month ago

Because it's new in C/igraph 0.10. I kept insisting that R/igraph 2.0 should have exposed this argument for all adjacency matrix functions. Doing this was going to be quite a bit of work though, and requires some internal refactoring. In the end there was no time.

One of the difficulties is that some adjacency matrix conversions were implemented in pure R. These should be moved to use C. That in turn requires a proper implementation for conversions between R and C sparse matrices. That's where this got stuck.

Here's a first step: https://github.com/igraph/rigraph/pull/1063