Skip to content

Fix issues with edge_index#13

Open
fjwong wants to merge 3 commits intoupphiminn:masterfrom
fjwong:fix-edge-index
Open

Fix issues with edge_index#13
fjwong wants to merge 3 commits intoupphiminn:masterfrom
fjwong:fix-edge-index

Conversation

@fjwong
Copy link
Copy Markdown

@fjwong fjwong commented Mar 31, 2020

This fixes a bug when creating the induced graph, in which the first edge of the induced graph (with index 0) can end up being duplicated instead of having its weight updated.

@fjwong
Copy link
Copy Markdown
Author

fjwong commented Mar 31, 2020

By the way, thank you for making this library.
I've been using it a lot in one of my projects.

} else {
edge_index[key] = graph.edges.length;
graph.edges.push(edge);
edge_index[edge.source + '_' + edge.target] = graph.edges.length - 1;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand why you think this is wrong? Indexes shouldn't go up to length; otherwise the other changes look fine :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this logic is not wrong at all. It's just that subtracting 1 seemed a bit redundant since we can get the same value from graph.edges.length prior to adding the edge to graph.edges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants