Skip to content

Allow correct creation of TemporalGraph with isolated nodes#321

Closed
vineetbansal wants to merge 5 commits into
devfrom
vb/isolated_nodes
Closed

Allow correct creation of TemporalGraph with isolated nodes#321
vineetbansal wants to merge 5 commits into
devfrom
vb/isolated_nodes

Conversation

@vineetbansal

@vineetbansal vineetbansal commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

The code supposedly allows creation of isolated nodes in a TemporalGraph, using:

t = TemporalGraph.from_edge_list([("a","b",1),("b","c",5)], num_nodes=4)

but then later would have raised an error:

print(list(t.nodes))  # IndexError raised

This is because from_edge_list built the IndexMap from the edge list alone, so it only contained IDs for a, b and c. num_nodes was passed straight through to Data without being reconciled against that mapping, so the graph reported n == 4 while only 3 nodes had IDs.

This PR first demonstrates the issue as 3 xfail tests in the first commit, and then fixes the underlying issue in the second commit, removing the xfails.

M-Lampert and others added 5 commits July 9, 2026 18:10
)

* fix ffmpeg and update versions

* Fix doc_string change detection

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* add network pytest marker to make netzschleuder tests CI compatible

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@vineetbansal

Copy link
Copy Markdown
Collaborator Author

TODO: Check if this problem exists in the base Graph and fix it there if possible.

@vineetbansal
vineetbansal changed the base branch from main to dev July 16, 2026 15:06
@vineetbansal

Copy link
Copy Markdown
Collaborator Author

Closing this in favor of another PR coming soon that targets the base Graph class instead.

@M-Lampert

Copy link
Copy Markdown
Contributor

Just remembered that we had related issues (in Graph) before in #277.
You can find snippets from our discussion below:

We are happy to know that you are using pathpyG and appreciate your effort in helping us improve it.

The problem is that when using from_edge_list to create a graph, we assign node_IDs to each node that are given in the list. In the example, this is a, b and c. However, the two isolated nodes that should be in the graph (according to num_nodes) don't have a node_ID, resulting in the IndexError that is mentioned.

While this is not the intended behaviour, there is no clear solution to solve this. I think we should remove the option to assign a number of nodes from the method from_edge_list so that it is not possible to assign node_IDs to only some of the nodes in the graph and ensure that either all or no nodes have an ID. Otherwise, we would need to assign a default node ID to nodes that are not mentioned in the edge_list.
If we remove the option num_nodes from from_edge_list, it would still be possible to create a graph with isolated nodes by including an IndexMap as follows:

Graph.from_edge_list(edge_list=[[0, 1], [0, 2]], mapping=IndexMap(["a", "b", "c", "d", "e"]))

@IngoScholtes what do you think?

Originally posted by @M-Lampert in #263

With @IngoScholtes reply:

I agree with this solution, i.e. different from the function Graph.from_edge_index there should not be a num_nodes parameter for the function Graph.from_edge_list.

I removed the parameter in the associated branch, which also required some changes in other functions that previously used the parameter. All tests are passing.

With the new implementatio, the suggested solution for the example above would be:

> g = pp.Graph.from_edge_list([('a', 'b'), ('a', 'c')], mapping=pp.IndexMap(list('abcde')))
> print(g)

Directed graph with 5 nodes and 2 edges

I think we should handle it the same in temporal graph.

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