cytoscape / cytoscape.js

Graph theory (network) library for visualisation and analysis
https://js.cytoscape.org
MIT License
10.09k stars 1.64k forks source link

Documentation: cy.json() requires edge id attribute, while normal graph initialization does not #2805

Closed simonwuelker closed 3 years ago

simonwuelker commented 3 years ago

Environment info

Current (buggy) behaviour If the edges passed to cy.json() do not contain an id attribute they are not created. I didnt find this mentioned anywhere and the Documentation(https://js.cytoscape.org/#notation/elements-json) specifically states that the id attribute may be omitted at initialization (and from what i understand, updating a graph using cy.json and initializing it are basically the same thing) There is also no error whatsoever.

FYI: This issue stems from a networkx issue of mine

Desired behaviour If the behavior described aboved is intentional i expect Cytoscape.js to throw some kind of error if i try to update the graph without edge id. It would also be nice to mention that somewhere in the Documentation.

If it is unintentional i would expect Cytoscape.js to consider all edges without ids to be new edges.

Minimum steps to reproduce Initialize a Graph and try to update it using cy.json({elements: {nodes:[ ... ], edges: [ ... ]}}) without providing edge IDs Take a look at this Example. I consider this either an unintentional bug or a lack of Documentation as the cytoscape demo tutorial information states that the id attribute is inferred automatically if omitted.

maxkfranz commented 3 years ago

I get where you're coming from, and I'll give a bit of context: cy.json() is meant for specifying the graph declaratively. In this case, you need to be able to identify which elements are specified. The canonical way to do that is with IDs, and all elements must have IDs anyway. This isn't an issue for ele.json(), where you already know which element you're talking about.

There are other approaches than ID, e.g. using a strict ordering. Different approaches have different tradeoffs. If you're interested in developing other approaches in a pull request, you're welcome to discuss your ideas for that here to get feedback.

As you mentioned, another option would be to improve the documentation. Would you be open to making a pull request to update the markdown text for cy.json()? A complementary option would be to have a clear error message in cy.json().

simonwuelker commented 3 years ago

Sure, i can do that. Just a spontaneous idea: Can't we just assume that any edges passed to cy.json() without IDs are new ones and add them to the Graph? (Basically treating them as if they had an ID that doesnt match any node in the Graph)This seems like the easiest solution to me and would probably only require very slight modifications in the existing code.

maxkfranz commented 3 years ago

Sure, i can do that. Just a spontaneous idea: Can't we just assume that any edges passed to cy.json() without IDs are new ones and add them to the Graph? (Basically treating them as if they had an ID that doesnt match any node in the Graph)This seems like the easiest solution to me and would probably only require very slight modifications in the existing code.

That might only work well when you start with an empty network. Generally, it could be confusing. You might specify the same edges in a subsequent cy.json() call and expect the edges to remain the same. However, you'd get duplicate edges with just the IDs differing.

maxkfranz commented 3 years ago

In the case where you want to do a one-time import, it may be better to use cy.add().

If you're looking to update/diff an existing graph, it's correct to use cy.json(). However, you need to have element IDs.