multiple edges not treated correctly

[SvenSchiffner]

At the moment I write test for the methods of the graph class and discovered two issues that's not easily solveable for me without to rewrite a lot of stuff. I use the fixed version from #176.

First multiple_edges are treated like directed edges. This isn't correct in my context.

from pprint import pprint
from ipycytoscape.cytoscape import Graph

data = {
    "nodes": [
        { "data": { "id": "0"} },
        { "data": { "id": "1"} },
        { "data": { "id": "2"} },
    ],
    "edges": [
        {"data": { "source": "0", "target": "1" } },
        {"data": { "source": "1", "target": "0" } },
        {"data": { "source": "1", "target": "2" } },
        {"data": { "source": "2", "target": "0" } },
    ]
}

graph = Graph()
graph.add_graph_from_json(data, multiple_edges=True)
pprint(graph.edges)

Will give us

[Edge(classes=' multiple_edges ', data={'source': '0', 'target': '1'}),
 Edge(classes=' multiple_edges ', data={'source': '1', 'target': '0'}),
 Edge(classes=' multiple_edges ', data={'source': '1', 'target': '2'}),
 Edge(classes=' multiple_edges ', data={'source': '2', 'target': '0'})]

This is a bug in my opinion but not so important.

But If you want to remove the edges

graph.remove_edge_by_id("0", "1")
pprint(graph.edges)

[Edge(classes=' multiple_edges ', data={'source': '0', 'target': '1'}),
 Edge(classes=' multiple_edges ', data={'source': '1', 'target': '2'}),
 Edge(classes=' multiple_edges ', data={'source': '2', 'target': '0'})]

You will see that the wrong edge was removed, but this issue you can fix with a small modification. But if you now try to remove the other edge a critical bug occurs

graph.remove_edge_by_id("1", "0")
pprint(graph.edges)

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-104-edc66acb34a7> in <module>()
----> 1 graph.remove_edge_by_id("1", "0")
      2 pprint(graph.edges)

<ipython-input-78-1df3dbb042ff> in _remove_edge_by_id(self, source_id, target_id)
    150             edge_id = i
    151     if edge_id != -1:
--> 152         self.remove_edge(self.edges[edge_id])
    153     else:
    154         raise ValueError(

<ipython-input-78-1df3dbb042ff> in _remove_edge(self, edge)
    124     try:
    125         self.edges.remove(edge)
--> 126         del self._adj[edge.data['source']][edge.data['target']]
    127         if 'directed' not in edge.classes:
    128              del self._adj[edge.data['target']][edge.data['source']]

KeyError: '1'

self._adj is an object that is in my opinion synced with the fronted and I know nearly nothing about javascript or typescript. I don't know if we can get rid of it or how important it is. I also don't know if the edge should occur multiple times it exits multiple times. What is your opinion @ianhi? I think you wrote a lot of the typescript stuff.

Maybe you can think that this is only an edge case. But here is another example where the same bug occurs

from pprint import pprint
from ipycytoscape.cytoscape import Graph

data = {
    "nodes": [
        { "data": { "id": "0"} },
        { "data": { "id": "1"} },
        { "data": { "id": "2"} },
    ],
    "edges": [
        {"data": { "source": "0", "target": "1", "weight": "1" } },
        {"data": { "source": "0", "target": "1", "weight": "2" } },
        {"data": { "source": "1", "target": "2" } },
        {"data": { "source": "2", "target": "0" } },
    ]
}

graph = Graph()
graph.add_graph_from_json(data, multiple_edges=True)
pprint(graph.edges)

graph.remove_edge_by_id("0", "1")
pprint(graph.edges)

graph.remove_edge_by_id("0", "1")
pprint(graph.edges)

I this case you can't remove the egde with the weight 2.

[marimeireles]

Actually, @MridulS was the one who introduced the adjacency list. Is it possible that you have a look at this?

[marimeireles]

And thanks a lot for finding this issue @sven5s! When we fix this we should write a test that test exactly these examples that you wrote.

[SvenSchiffner]

I will take a look but if it's connected to typescript I need some help. These code snippets are parts from my actual tests. ;-)

[SvenSchiffner]

Okay, I wrapped my head around this and found the problem for the error. I also understand the function of _adj and it isn't connected to typescript. _adj will also help us to handle multiple_edges the right way. But first I want to reduce the code base a little #195.

My plan is to get my PR #176 merged. Than I will create a PR for #195 and if all problems are solved (another PR). I will make a PR for the tests, because maybe some things change along the way and I don't want to rebase a thousand times. After this we can continue with #162 and maybe change the node and edge classes or simply implement the classes attribute as a list how it would be useful. So we have a good working and tested base and can continue to make deeper changes.

[ianhi]

My plan is to get my PR #176 merged. Than I will create a PR for #195 and if all problems are solved (another PR). I will make a PR for the tests, because maybe some things change along the way and I don't want to rebase a thousand times. After this we can continue with #162 and maybe change the node and edge classes or simply implement the classes attribute as a list how it would be useful.

This is an excellent plan :)

I'm also always happy to help discuss how the typescripty parts work, feel free to ask questions on modifying them or why they are the way they are.

[jenshnielsen]

Looks like this can be closed since #225 is merged?