Broken graph state after removing node

[ktaletsk]

Bug report

cytoscapeobj.graph.remove_node(node) removes more nodes than just the node passed as the parameter. This also affects cytoscapeobj.graph.remove_node_by_id(node_id) which uses it.

ipycytoscape/ipycytoscape/cytoscape.py

Lines 287 to 305 in c72f39d

	def remove_node(self, node):
	"""
	Removes node from the end of the list. Equivalent to Python's remove method.
	Parameters
	----------
	node : ipycytoscape.Node
	"""
	try:
	self.nodes.remove(node)
	for target in list(self._adj[node.data["id"]]):
	self.remove_edge_by_id(node.data["id"], target)
	for source in list(self._adj):
	for target in list(self._adj[source]):
	if target == node.data["id"]:
	self.remove_edge_by_id(source, node.data["id"])
	del self._adj[node.data["id"]]
	except ValueError:
	raise ValueError(f'{node.data["id"]} is not present in the graph.')

Issues seems to lie specificly in this call:

ipycytoscape/ipycytoscape/cytoscape.py

Line 296 in c72f39d

self.nodes.remove(node)

Code for reproduction

1. Open 'DAG example' notebook.

2. Execute all existing cells in order. The graph would look like this
   

3. Observe the list of existing nodes

cytoscapeobj.graph.nodes
_______________________
[Node(data={'id': 'n0'}, position={}),
 Node(data={'id': 'n1'}, position={}),
 Node(data={'id': 'n2'}, position={}),
 Node(data={'id': 'n3'}, position={}),
 Node(data={'id': 'n4'}, position={}),
 Node(data={'id': 'n5'}, position={}),
 Node(data={'id': 'n6'}, position={}),
 Node(data={'id': 'n7'}, position={}),
 Node(data={'id': 'n8'}, position={}),
 Node(data={'id': 'n9'}, position={}),
 Node(data={'id': 'n10'}, position={}),
 Node(data={'id': 'n11'}, position={}),
 Node(data={'id': 'n12'}, position={}),
 Node(data={'id': 'n13'}, position={}),
 Node(data={'id': 'n14'}, position={}),
 Node(data={'id': 'n15'}, position={}),
 Node(data={'id': 'n16'}, position={})]

4. Remove node with id n3 (the bottom-right child node in the left-most graph)

cytoscapeobj.graph.remove_node_by_id('n3')

The graph now looks like this:

Alternatively, you can remove the node from the list of nodes (which is what cytoscapeobj.graph.remove_node does internally) to the same effect:

cytoscapeobj.graph.nodes.remove(cytoscapeobj.graph.nodes[3])

5. Inspect the list of nodes

cytoscapeobj.graph.nodes
_______________________
[Node(data={'id': 'n0'}, position={}),
 Node(data={'id': 'n1'}, position={}),
 Node(data={'id': 'n2'}, position={}),
 Node(data={'id': 'n4'}, position={}, removed=True),
 Node(data={'id': 'n5'}, position={}, removed=True),
 Node(data={'id': 'n6'}, position={}, removed=True),
 Node(data={'id': 'n7'}, position={}, removed=True),
 Node(data={'id': 'n8'}, position={}, removed=True),
 Node(data={'id': 'n9'}, position={}, removed=True),
 Node(data={'id': 'n10'}, position={}, removed=True),
 Node(data={'id': 'n11'}, position={}, removed=True),
 Node(data={'id': 'n12'}, position={}, removed=True),
 Node(data={'id': 'n13'}, position={}, removed=True),
 Node(data={'id': 'n14'}, position={}, removed=True),
 Node(data={'id': 'n15'}, position={}, removed=True),
 Node(data={'id': 'n16'}, position={}, removed=True)]
...

6. JS console logs indicate an error immediately after running cytoscapeobj.graph.remove_node_by_id('n3'):

Uncaught (in promise) Error: Can not create second element with ID `n4`
Uncaught (in promise) Error: Can not create second element with ID `n4`
Uncaught (in promise) Error: Can not create edge `a1b4ae96-fe81-4363-ba59-23298b1a10e3` with nonexistant source `n4`

Is this the intended behavior?

Actual outcome

Nodes with ids "higher" than node requested to be deleted are getting removed=True on them and disappear from the graph.

Expected outcome

Maybe it would make more sense to make this optional, and only delete a single node by default?

Version Info

• runtime environment: Binder (link above)
  • ipycytoscape version : 1.3.2
  • Python version: 3.7.12
  • JupyterLab version: 3.3.1

[ktaletsk]

BTW, same error with edges:

In the same 'DAG exampe' notebook, run

cytoscapeobj.graph.remove_edge_by_id('n1', 'n3')

cytoscapeobj.graph.edges
_______________________
[Edge(data={'source': 'n0', 'target': 'n1'}),
 Edge(data={'source': 'n1', 'target': 'n2'}),
 Edge(data={'source': 'n4', 'target': 'n5'}, removed=True),
 Edge(data={'source': 'n4', 'target': 'n6'}, removed=True),
 Edge(data={'source': 'n6', 'target': 'n7'}, removed=True),
 Edge(data={'source': 'n6', 'target': 'n8'}, removed=True),
 Edge(data={'source': 'n8', 'target': 'n9'}, removed=True),
 Edge(data={'source': 'n8', 'target': 'n10'}, removed=True),
 Edge(data={'source': 'n11', 'target': 'n12'}, removed=True),
 Edge(data={'source': 'n12', 'target': 'n13'}, removed=True),
 Edge(data={'source': 'n13', 'target': 'n14'}, removed=True),
 Edge(data={'source': 'n13', 'target': 'n15'}, removed=True)]

Clearly, there is an issue calling remove method of ipycytoscape.cytoscape.Graph.nodes and ipycytoscape.cytoscape.Graph.edges (both of the spectate.models.List type)

[marimeireles]

Hey @ktaletsk, thanks for opening the issue and sorry for taking a while to reply to it.
The person who contributed this code was @MridulS, maybe they're interested in picking this up? Or just chiming in.
But anyway, that's a great found! Thank you very much for contributing with the issue, lately I haven't had much time to hack on ipycytoscape, but this issue is definitely up in my list.
I can try to help you if you're interested in tackling it. I saw you're in LA? I'm here for the week! If you want we could even meet in real life.
Cheers.

[MridulS]

Yeah this doesn't look right, thanks for the report.

I don't have much bandwidth for this right now but I can get back to this in a couple of weeks :) Feel free to submit a PR to fix this if you get the time before that.

[ktaletsk]

@marimeireles @MridulS here are some findings upon debugging the issue

1. It only manifests itself if widget is rendered. Consider these 3 examples based on the code from the test suite:
   • Initial setup

     from ipycytoscape.cytoscape import Edge, Graph, Node, CytoscapeWidget
     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": "0"}},
             {"data": {"source": "1", "target": "2"}},
             {"data": {"source": "2", "target": "0"}},
         ],
     }
     

   • Create graph, removed node. Works as expected

     graph = Graph()
     graph.add_graph_from_json(data)
     graph.remove_node_by_id("0")
     graph.nodes
     >> [Node(data={'id': '1'}, position={}), Node(data={'id': '2'}, position={})]
     

   • Same thing, but inside the CytoscapeWidget object. Still works

     widget = CytoscapeWidget()
     widget.graph = Graph()
     widget.graph.add_graph_from_json(data)
     widget.graph.remove_node_by_id("0")
     widget.graph.nodes
     >> [Node(data={'id': '1'}, position={}), Node(data={'id': '2'}, position={})]
     

   • Exactly the same thing, but render the widget before deleting the node. Then we observer the bug behavior

     widget = CytoscapeWidget()
     widget.graph = Graph()
     widget.graph.add_graph_from_json(data)
     
     # Render widget
     widget
     
     widget.graph.remove_node_by_id("0")
     widget.graph.nodes
     >>[Node(data={'id': '1'}, position={}, removed=True), Node(data={'id': '2'}, position={}, removed=True)]
     

With that, I think we should search for an error somewhere in the Jupyter widget.

2. It is not completely clear to me what's happening in CytoscapeView.removeNodeView and CytoscapeView.removeEdgeView

removeNodeView(nodeView: any) {
    nodeView.model.set('removed', true);
    nodeView.remove();
  }

I tried commenting out setting the model - the first line in the function - and the behavior was better, yet still incorrect. I was able to remove only the requested node from the list, and the state of the graph was as expected - no nodes with incorrectly set "removed: true". However, the widget was not updated automatically and I had to manually rerun the cell with the widget to get the new state.

I would like to continue debugging, but I have 2 issues:

1. What is the type of the nodeView input argument in CytoscapeView.removeNodeView? It is set to any which makes it impossible to inspect the methods of this object.
2. Who and where is calling CytoscapeView.removeNodeView?

[ktaletsk]

@marimeireles @MridulS I would like to fix this issue, but I'm stuck. Could you give some feedback on my comment above?

[vlvalenti]

I have run into this issue as well trying to update ipycytoscape widgets rendered in Jupyter. Removing any single node removes all the nodes rendered in the graph and sets each node to removed=True. Setting the nodes back manually to removed=False does not re-add them. Is there intention to address this bug?