Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple edges between nodes #185

Open
aj47 opened this issue Mar 22, 2019 · 37 comments
Open

Multiple edges between nodes #185

aj47 opened this issue Mar 22, 2019 · 37 comments
Assignees
Labels
enhancement improving existent functionality or performance related in progress needs more info not enough proof to specify issue priority high

Comments

@aj47
Copy link

aj47 commented Mar 22, 2019

Describe the bug
I'm trying to have multiple edges between the same two nodes but i get a same key waring and no extra edges rendered. how can i get around this?

Warning:

Warning: Encountered two children with the same key, `Trump,order`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.
@grant37
Copy link
Contributor

grant37 commented Mar 22, 2019

I was also looking at this. Seems the key for a link is always created from the source and target node ids in graph.renderer.jsx. You can see it in the _renderLinks function around line 37:

const sourceId = source.id !== undefined && source.id !== null ? source.id : source;
const targetId = target.id !== undefined && target.id !== null ? target.id : target;
const key = \`${sourceId}${CONST.COORDS_SEPARATOR}${targetId}\`;

So I think avoiding the react issues mentioned in the key warning would require a change to that line setting the key. Maybe including the link.labelProperty in the key or using a link.id property if one was added to the link config. Anything I guess that would allow creating a unique link id for any link that gets passed into the Graph via data.links array prop.

But I think because the actual svg links seem to be drawn from the source and target node origins two unique links between the same nodes would still render on top of one another. Not sure that’s true or not. And being unfamiliar with d3 not sure if it would be easy to change or not. Maybe curved links would show up if their paths are somewhat random?

@aj47
Copy link
Author

aj47 commented Mar 24, 2019

Thanks for the insights grant, yeah it seems possible with d3 js as per this jsfiddle i found on stack overflow http://jsfiddle.net/7HZcR/3/

@danielcaldas
Copy link
Owner

Yes, currently we can only have one link between two nodes. We can work to expand this functionality, I see two alternatives:

  1. We generate an internal identifier that avoids collisions appending some kind of count to the source - target, react-d3-graph would have to keep track of links to detect duplicates and generate unique key values.
  2. The user provides within each link something unique that could be used to generate the key alongside with the source and target properties.

To be honest option 1. does not look bad at all... What do you guys think @aj47 @grant37 @LonelyPrincess @wendymungovan

@danielcaldas danielcaldas added the needs more info not enough proof to specify issue label Mar 27, 2019
@wendymungovan
Copy link

I think option 2 will make it easier when the user makes updates to the existing graph data. This makes them responsible for keeping track of which link is which and not the library.

@danielcaldas
Copy link
Owner

Good point @wendymungovan, it's also easier for clients to the scale the complexity accordingly if they are in control, given this option 2 is also very valid, but I would not yet discard option 1. Let's see what do the others think about it?

@grant37
Copy link
Contributor

grant37 commented Mar 29, 2019

Hmm I guess each has its advantages. I think @wendymungovan is right that the second option would be a bit easier to implement. And the user might off-hand have a simple way to generate unique ids based on their data that doesn't use the index of the link in the links array. Because my initial thought was that the indices would make option 1 pretty simple but then I guess you want to avoid using indices for uniqueness in keys when the order of elements isn't static. @danielcaldas did you have an idea in mind for managing the keys internally in the first option? Anyway, I'd be open to whichever you all feel will work.

@LonelyPrincess
Copy link
Collaborator

LonelyPrincess commented Mar 29, 2019

I agree with @wendymungovan in that we should give users a way to handle those links themselves. I think it's better to give the users as much control as possible, so if I'd had to chose, I'd go with option 2 as well.

As for the first option, I can't think of a way to implement it, because keeping track of the changes when the user modifies the data doesn't seem possible unless the user himself has provided some kind of id in the first place.

We could do what you say and assign each link an internal identifier by using the linked nodes and a counter, but... How would that allow us to identify which is which when the data changes? If the user changes the color for one of the links between two nodes A and B, how can we know for sure if the change happened into A-B-1 or A-B-2? 🤔

When we create the internal id we're only considering the value of the source and target properties, but multiple nodes can share that. We could append a counter at the end, but that value will depend entirely on the order in which the links appear inside of the data object. If the user removes one of the links between a set nodes or just changes the order of the items in the links array, our internal ids could be no longer be pointing to the right place.

Having said that, it seems the more logical to make the users responsible of managing the identifiers of their links, as they already do with the nodes. What do you think, @danielcaldas ?

@danielcaldas
Copy link
Owner

From your feedback it seems that option 2 would be our best chance, I also like it and it will help us keep the codebase clean and generic 🙂

Another thing to bear in mind is that I think this should not be an opt-in feature. Once we define in react-d3-graph API that clients must provide unique identifiers within link payload, this will no longer be compatible with the current API that we have within v2 of the library. I will, for this reason, deprioritize this issue and try to get as much as bug fixes and other non-impacting features into v2 of the library also given that this one is quite recent.

@stale stale bot added the wontfix label May 10, 2019
@stale stale bot closed this as completed May 30, 2019
@danielcaldas danielcaldas reopened this May 30, 2019
@stale stale bot removed the wontfix label May 30, 2019
Repository owner deleted a comment from stale bot May 30, 2019
@liobouchan
Copy link

Hello i have the some issue relationed to this issue "Multiple edges between nodes #185", I have 2 nodes.

nodes:[
  {
    id: 1,
    name: "A"
  },
  {
    id: 2,
    name: "B"
  }
]

And i have multiple relationships beetween this nodes i have an unique id for each relationship , my links are like this.

links:[
    {id: 101, source: 1, type: "GO_TO", target: 2},
    {id: 102, source: 1, type: "HAVE", target: 2},
    {id: 103, source: 1, type: "SEND_TO", target: 2},
    {id: 104, source: 1, type: "PHONE_TO", target: 2}
]

I now that multiple links between nodes are not possible , but i have a problem, i got a lot of warning messages all the time when i touch the nodes or when i update the graph. The message is this:

Warning: Encountered two children with the same key, 1,2. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.
in g (created by Graph)
in svg (created by Graph)
in div (created by Graph)
in Graph (at Grapho.js:123)
in Grapho (at GraphViewer.js:146)
in div (created by Col)
in Col (at GraphViewer.js:145)
in div (created by Row)
in Row (at GraphViewer.js:144)
in div (at GraphViewer.js:143)
in GraphViewer (at DefaultLayout.js:57)
in Route (at DefaultLayout.js:51)
in Switch (at DefaultLayout.js:48)
in Suspense (at DefaultLayout.js:47)
in div (created by Container)
in Container (at DefaultLayout.js:46)
in main (at DefaultLayout.js:44)
in div (at DefaultLayout.js:33)
in div (at DefaultLayout.js:27)
in DefaultLayout (at App.js:30)
in Route (at App.js:30)
in Switch (at App.js:24)
in Suspense (at App.js:23)
in Router (created by HashRouter)
in HashRouter (at App.js:22)
in App (at src/index.js:10)

Issue

It affects a lot the performance on my graph, how can i fix this message? , it doesn't matter that i can have multiple edges between the nodes, but how to disable this message?, also i want to help in the task to support multiple links between nodes , i think that the best option is the option 2.

I want to code and help to solve this issue . Greetings from Mexico, sorry for my bad English i speak Spanish.

@danielcaldas
Copy link
Owner

Hello @liobouchan please use the issue/feature report template and open a new issue.

(But what I see from the logs, it's more of an internal error of React that is caused because you have the same identifier either for links or nodes.)

@liobouchan
Copy link

Thanks @danielcaldas , Thank you very much I will create the issue, what seems strange to me is that the links and nodes have completely different identifiers, I do not understand where that problem would come from.

@ghost
Copy link

ghost commented Aug 7, 2019

Any progress on this ? A workaround ?

@woozyking
Copy link

woozyking commented Sep 5, 2019

from #185 (comment)

or using a link.id property if one was added to the link config

this seems to be a solution utilized in similar libraries, such as react-sigma

@stale
Copy link

stale bot commented Nov 4, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 4, 2019
@stale stale bot closed this as completed Feb 2, 2020
@danielcaldas danielcaldas reopened this Mar 1, 2020
@akshgpt25
Copy link

@danielcaldas: Is this feature anywhere in your nearby target. Not able to use this library because of the same. Or can you provide some other alternative as of now

@grant37
Copy link
Contributor

grant37 commented Jun 9, 2020

@akshgpt25 you may be able to use this component, which renders a cytoscape graph.

@danielcaldas danielcaldas added enhancement improving existent functionality or performance related and removed priority low labels Jul 2, 2020
@wetas2020
Copy link

what is the solution in this case if someone wants to create multiple links between two nodes?

@Buba21
Copy link

Buba21 commented Jul 31, 2020

Hi is there any development regarding this particular issue? Or did anyone else find a workaround this?

@akshgpt25
Copy link

akshgpt25 commented Jul 31, 2020 via email

@Buba21
Copy link

Buba21 commented Jul 31, 2020

Although Cytoscape has multiple edges support it doesnt support some other features that this lib supports, which is unfortunate.

@akshgpt25
Copy link

akshgpt25 commented Jul 31, 2020 via email

@danielcaldas
Copy link
Owner

Related: #346

@danielcaldas danielcaldas added the breaking change a breaking change in the library label Aug 14, 2020
@Shrutijamgade
Copy link

Shrutijamgade commented Aug 17, 2020

We also faced similar issue where multiple links were to be created between 2 nodes. Regarding this there were 2 issues:

  1. We kept getting this warning.
    Warning: Encountered two children with the same key, Trump,order. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version
  2. After the first issue was solved, the links were overlapping each other.

To solve the first issue:
Since the graph was part of a react app, the react-d3-graph was inside node_modules directory. So we moved the module to our working directory and used it as component.

We changed the "key" value as mentioned by @grant37 to a link id. This resolved the first problem.

To solve the second issue, we added a "count" key in our links array which indicated the number of time that particular pair of source and target were occurring.

For Example:

[{'id':1 ,'label':'friend', 'source':'Harry', 'target':'Sally', 'count':1}, {'id':2, 'label':'classmate', 'source':'Harry', 'target':'Sally', 'count':2}, {'id':3, 'label':'roommate', 'source':'Alice', 'target':'Sally', 'count':1}]

We edited "buildLinkProps" and "smoothCurveRadius" function to pass the count value. The function looked as follows:

function smoothCurveRadius(x1, y1, x2, y2,count) { const dx = (x2 - x1) * count; const dy = (y2 - y1) * count; return Math.sqrt(dx * dx + dy * dy); }

My teammates were @GTroxx55 and @manjithd2!

@danielcaldas
Copy link
Owner

Hey @Shrutijamgade thanks for sharing!

To solve the second issue, we added a "count" key in our links array which indicated the number of times that particular pair of source and target were occurring.

That makes sense, if multiple links are rendered one would need to provide a weight/value on the connection between the same nodes so that they could be differentiated, this can then be used to compute the curvature of the link as you've illustrated 👍

@danielcaldas
Copy link
Owner

Hello all, I have a proposal here to support redundant links in the library, want to try it out? #362
@akshgpt25 @Shrutijamgade @andfelzapata @liobouchan

@QYuQianchen
Copy link

Hi all, do we have an update on this issue?

@danielcaldas
Copy link
Owner

Yes @chenchenyo, there's a proposal here #362 that it's waiting for feedback. Have a look and let me know if that's what you're expecting with this change.

@michaelbdavid
Copy link

Hey @danielcaldas,
I'd like to try out your work around listed above for multiple-link support. How can I pull this into my app?

Thanks,
Michael

@danielcaldas
Copy link
Owner

Hey @michaelbdavid you can npm install the feature branch directly into your app, just change your package.json as follow:

"react-d3-graph": "danielcaldas/react-d3-graph#feature/multiple-edges-between-same-nodes",

And follow with an npm install so that you fetch the branch version of the code.

@michaelbdavid
Copy link

michaelbdavid commented Oct 19, 2020 via email

@michaelbdavid
Copy link

michaelbdavid commented Oct 19, 2020 via email

@danielcaldas
Copy link
Owner

Also is there a way to have these multi-links spread out a little further?

Hey @michaelbdavid, I'll take this into consideration when looking at the feature again.

@ChuckJonas
Copy link

@danielcaldas any chance of releasing a beta/next build to npm? I'm currently working on a "spike" out of code sandbox and having this feature would make the upcoming demo go much better!

@danielcaldas
Copy link
Owner

@ChuckJonas still needs a bit more testing, and the implementation is not final yet. Hopefully, your demo was awesome regardless.

@eliltz
Copy link

eliltz commented Apr 11, 2021

Hi, Is there an update on this issue? Thanks

@noonation
Copy link

Related question I don't see addressed above. Is there any issue with COMPLEMENTARY EDGES, ie. where SOURCE1 = TARGET2 and SOURCE2 = TARGET1 ?

This is very common in social graphs of course, ie. two people follow each other.

Since the link key is a combination of SOURCE and TARGET, the two links would have different keys unless the key is order the same regardless of which is source and which is target.

So, can I have two edges A > B and B > A ?

@nch3ng
Copy link

nch3ng commented Apr 21, 2023

Any update on this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improving existent functionality or performance related in progress needs more info not enough proof to specify issue priority high
Projects
None yet
Development

No branches or pull requests