Remove interop with 'graph' package (or move to another package)

[szhorvat]

This issue is for discussing the removal of interoperability with the graph package, i.e. the as_graphnel() and graph_from_graphnel() functions.

The main question is: Is graph prominent and popular enough to warrant maintaining interop on the igraph side?

Concerns:

• graph is sloppy about the handling of non-simple graphs. It is not clear if it supports them, in what way they should be represented, etc.
• The development of graph does not appear to happen in the open. I was unable to find a public issue tracker or other place to coordinate the interop, ask for support, report issues.
• No response about the multigraph issue after multiple contact attempts, see https://support.bioconductor.org/p/9148438/#9153995 and https://github.com/Bioconductor/graph/issues/17
• There seems to be little graph development.
• graph is no longer on CRAN, making testing a bit more difficult, and suggesting (??) that it's not a tool intended for a general audience.

The conditions are not there for igraph to maintain interoperability functions that are up to the standards that I'd like to see igraph reach.

Opinions? @mbojan, your input would be valuable.

Does anyone have the GitHub handle of graph developers, who could be involved in this discussion? We need to figure out where the best place is for such conversion function: igraph, graph, or a third package?

References:

• https://github.com/cytoscape/RCy3/issues/202

[mbojan]

I see validity in these concerns, especially if it will allow igraph to drop graph as a dependency... graph is used to non-trivial extent within Bioconductor.

Does anyone have the GitHub handle of graph developers, who could be involved in this discussion?

https://github.com/Bioconductor/graph

We need to figure out where the best place is for such conversion function: igraph, graph, or a third package?

I made intergraph (https://github.com/mbojan/intergraph) just for that. We can consider moving these functions there.

[szhorvat]

I made intergraph (https://github.com/mbojan/intergraph) just for that. We can consider moving these functions there.

If you are willing to host the functions there in the long term, I think that would be a good idea. It would also improve igraph. Currently, these functions use internal functionality that is not exposed. They should be implementable with igraph's standard public functionality though, so moving them will force an improvement in the igraph API.

The decision is ultimately @krlmlr's though.

Does anyone have the GitHub handle of graph developers, who could be involved in this discussion?

https://github.com/Bioconductor/graph

@jwokaty @vjcitn @nturaga, you were the last committers to graph on GitHub. Your input would be helpful in making the igraph <-> graph conversion functions robust. Do you have any comments on https://github.com/Bioconductor/graph/issues/17 ?

[vjcitn]

We are working on this.

[vjcitn]

commit a99b4f69 to devel branch of https://git.bioconductor.org/packages/graph corrects the computation of degree for graphNEL with self-loops. This is version 1.79.2 of graph package. Unit tests are added, documentation not modified yet.

What else can we do to help with interop?

[szhorvat]

What else can we do to help with interop?

Thank for the offer of help @vjcitn !

What would be most useful is to know:

1. Does the graphNEL class support more than one connection between two vertices?
2. What about self-connections?
3. What is the correct way to construct the following graphs as a graphNEL object? Code examples for each would be helpful. If the answer to either point 1., or 2. is "no", then some of these shouldn't be constructed (i.e. the conversion function should throw an error).

graph1

graph2

graph3

graph4

[vjcitn]

Sorry for the delay in responding. graph3 is challenging. you intend these as "undirected" graphs, right?

Here is code for 1, 2, 4

library(graph)

library(Rgraphviz)

par(ask=TRUE)

g1 = new("graphNEL", nodes=c("1", "2", "3"), edgemode="undirected")

g1 = addEdge("1", "2", g1)

plot(g1)

g2 = new("graphNEL", nodes=c("1", "2", "3"), edgemode="undirected")

g2 = addEdge("1", "2", g2)

g2 = addEdge("2", "3", g2)

g2 = addEdge("3", "3", g2)

plot(g2)

g4 = new("graphNEL", nodes=c("1", "2", "3"), edgemode="undirected")

g4 = addEdge("1", "2", g4)

g4 = addEdge("2", "3", g4)

g4 = addEdge("3", "3", g4)

 g4@edgeL$`3`$edges = c(2,3,3)  # THIS WAS OMITTED IN PREVIOUS EDIT

plot(g4)

[szhorvat]

you intend these as "undirected" graphs, right?

Yes.

But what we need most is answers to questions above, i.e. is there self-loop support? I assume yes. Is there multigraph support? I assume no, i.e. igraph should reject multigraphs when converting to graphNEL. Basically we need to know which kinds of non simple graphs are representable by graphNEL (if any) and what the correct representation is (e.g. do we include self-loops once or twice in the adjacency list with undirected graphs)?

I attempted to edit your post above and add code blocks for readability, but this is not possible when responses were sent by email. Looking at the edit history, I don't think I broke anything, but the sample code for graph2 and graph4 look identical.

[vjcitn]

g4 code was not correct in the comment above, I edited it to reflect the double self-loop.

Self-loops are supported by graphNEL in the manner shown. It would be good to have a collection of examples to verify that the appropriate behavior is present, and to have a more idiomatic way of dealing with the double self-loop than is shown above. It would require modification to addEdge method, I think.

There is a separate class for multigraphs. It does not appear to have much mileage. For now, declining to convert multigraphs from igraph to graph makes sense.

[szhorvat]

g4 code was not correct in the comment above, I edited it to reflect the double self-loop.

Thanks! GitHub originally identified part of that code as an email address and redacted it. But g4 contains multi-edges (a multi-self-loop to be specific) so this would also be something we reject then.

[krlmlr]

Do we need to take further action here?

[vjcitn]

I don't know. I noticed your comment on Seurat indicating large-scale upgrades to igraph. Are there interop steps needed in graph to help answer this question? Should I test against a given branch of igraph?

[krlmlr]

The main branch of this repo is good now, almost a release candidate. I don't understand "interop steps".

[vjcitn]

As I understand this thread, the question is whether igraph should include any functions that involve passage of information between igraph and bioconductor's graph package. I will look at the latest draft of igraph to see whether this is still an issue.

[szhorvat]

I think it makes sense to maintain the status quo and get back to this after the 2.0.0 release.

We now have a direct line of communication, so we can (and mostly have) clarified issues like multigraph handling. We can make igraph throw an error during conversion when the graph package cannot represent the igraph graph.

[krlmlr]

2.0.1 is out now. What are the next steps here?