-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
trophic_levels now checks for paths from each node to a basal node #7453
base: main
Are you sure you want to change the base?
trophic_levels now checks for paths from each node to a basal node #7453
Conversation
I added the function
Following code was used to check the proper functioning of the feature. edges = [(0, 5), (0, 3), (0, 33), (3, 2), (3, 24), (3, 32), (33, 1), (33, 10), (2, 3), (2, 16), (1, 33), (10, 16), (16, 4), (16, 9), (18, 11), (11, 5), (11, 21), (4, 2), (4, 33), (9, 16), (9, 26), (9, 17), (26, 19), (17, 10), (19, 10), (19, 9), (19, 7), (19, 0)]
G = nx.DiGraph(edges)
res = nx.trophic_levels(G)
print(res[0]) Result before changes: Result after changes: |
This code is written to address issue #6899. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
I think the bulk of this could call the existing nx.bfs_layers
function which takes a parameter for multiple source nodes.
See below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance this seems to be on the right track, thanks! This should also have some test cases to demonstrate the changed/fixed behavior in trophic_levels!
reachable_nodes = { | ||
node | ||
for layer in nx.bfs_layers(graph, sources=basal_nodes) | ||
for node in layer | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote to use this directly on L59 and do away with the extra function definition - eliminating indirection makes the code much easier to follow IMO!
I will try to add the test cases. |
I include a comment with the term Fixes #6899 so that merging this PR automatically closes that issue. |
I think the tests actually passed -- it was a red X because one test got canceled, but rerunning it allowed it to pass. A second test could be a graph that works plus an isolated node. That will trigger the other exception added in this PR. |
This looks like a reasonable approach. The latest dev version of
Should a more clear error message be given for the null graph? |
Are you referring to the test case I added and then removed? |
Yes -- I was referring to the test added and then removed. That tested the "no basal nodes" exception. |
Should I add it again then? |
Yes -- could you add it again, and a test for a network that should work, but with an added isolated node so that it doesn't work -- and raises an exception from the 2nd code-check. Does this make sense? |
I tried adding a test for isolated nodes. But the test failed. Should I add a condition to handle isolated nodes? Or is there something I am missing here? Test: def test_trophic_levels_with_isolated_node():
G = nx.DiGraph()
# Adding edges to create a graph with a basal node
edges = [(0, 1), (1, 2), (2, 3)]
G.add_edges_from(edges)
# Adding an isolated node
G.add_node(4)
print(G.nodes)
with pytest.raises(nx.NetworkXError):
nx.trophic_levels(G) |
Fixes #6899