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

Implementing iterative removal of non_terminal_leaves in Steiner Tree approximation #7422

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

OrionSehn-personal
Copy link

This should resolve #6881

Might not be the fastest possible solution, it checks the neighbors of non-terminal leaves, and removes them in bunches of sets using set comprehension and differences.

I added a relatively simple test to ensure that this function acts as intended, and trims down any non-terminal leaves, but any consequential non-terminal leaves created by that trimming.

This might slow down the approximation significantly however, I didn't do speed testing on this, and I'm not sure if we should consider adding an optional flag in the Steiner tree approximation to perform this additional optimization.

@OrionSehn-personal OrionSehn-personal changed the title Implementing iterative removal of non_terminal_leaves #6881 Implementing iterative removal of non_terminal_leaves Apr 18, 2024
@OrionSehn-personal OrionSehn-personal changed the title Implementing iterative removal of non_terminal_leaves Implementing iterative removal of non_terminal_leaves in Steiner Tree approximation Apr 18, 2024
@rossbar
Copy link
Contributor

rossbar commented Apr 18, 2024

@OrionSehn there is already a PR open for this issue at #7136 with a fair bit of discussion - perhaps you could take a look at that one and weigh in there?

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

This approach makes sense to me and the test adequately demonstrates the problem with the current implementation (i.e. non-terminal nodes along a path become terminal with subsequent removals).

There are potential "gotchas" related to this approach implicitly assuming that leaf nodes have degree one. This may not always be true: two cases I can think of are 1) graphs with self-loop edges (see example) and 2) MultGraphs with multi-edges.

# Desired behavior
>>> G = nx.path_graph(10)
>>> _remove_nonterminal_nodes(G, [4, 5, 6])
>>> list(G)
[4, 5, 6]
>>> H = nx.path_graph(10)
>>> H.add_edge(9, 9)  # self-loop on one terminal node
>>> _remove_nonterminal_edges(G, [4, 5, 6])
>>> list(G)
[4, 5, 6, 7, 8, 9]

Since this function is only to be used in the context of the steiner_tree approximations, these may be fine - but it's worth double-checking how steiner_tree handles these cases to make sure the _remove_nonterminal_leaves gives consistent results for these cases!

OrionSehn and others added 2 commits May 21, 2024 17:00
improve test so that there are multiple non-terminal nodes on both ends of the path

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Steiner tree approximation does not iteratively remove nonterminal leaves
3 participants