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

Make dfs_predecessors and dfs_successors return iterator #1826

Closed
wants to merge 1 commit into from

Conversation

MridulS
Copy link
Member

@MridulS MridulS commented Oct 30, 2015

No description provided.

@dschult
Copy link
Member

dschult commented Dec 21, 2015

I think this latest version has trouble when the successors of a node is a list of more than one node. But the tests don't cover that case.

It would probably be good to change the test graph by adding a node so that some node has more than one successor. e.g. Instead of G consisting of 0->1->2->4->3->1 Maybe we should add 1->5 and adjust all the tests as needed. With the current test graph we test that DFS ends, but not that it has to traverse downward after retracing back up the tree.

As it is the suggested graph: 0->1->2->4->3->1->5 only has successor [5] instead of [2,5]

To get the dfs_successors to work without storing an entire dict and then iterating through it (which defeats the purpose of an iterator), I think we'll need to work with the dfs_labeled_edges building lists and looking for a "reverse" edge so that we know we're done with a node. Once we are done with a node we can yield it with children and delete the children from our internal storage list to free memory. This'll be a nice exercise in understanding DFS. :)

@MridulS
Copy link
Member Author

MridulS commented Dec 22, 2015

@dschult Yes the tests should be changed and more thorough. Will do that.

Let me know if this implementation makes sense

def dfs_successors(G, source):
    parent = source
    children = []
    for p, c, d in sorted(dfs_labeled_edges(G, source), key = lambda x: x[0]):
        if p == parent and d['dir'] == 'reverse':
           # this check should be removed with something more efficient 
            if c == p:
                continue
            children.append(c)
            continue
        elif p != parent:
            yield (parent, children)
            children = []
            parent = p
    yield (parent, children)

@MridulS
Copy link
Member Author

MridulS commented Dec 22, 2015

Well the above implementation isn't very helpful.
We can make it better just by rewriting sorted(dfs_labeled_edges(G, source), key = lambda x: x[0]) as sorted(nx.dfs_edges(G, source). They are basically doing the same things, and we are defeating the whole point of creating an iterator. By using sorted we are creating a list hence using up memory. Let me know what you think.
Thanks :)

@hagberg hagberg added this to the networkx-future milestone Dec 27, 2015
@dschult
Copy link
Member

dschult commented Jan 2, 2016

Sorting the dfs ordering defeats the point of requesting the dfs order. You won't get the successors in the requested order.

Maybe the first step in this is a PR that adds to the tests for dfs_successors. We should test with a graph that makes sure the order is done correctly. Once that PR is merged we can make iterators and be confident they are actually creating the dfs_successors.

@MridulS
Copy link
Member Author

MridulS commented May 18, 2022

I'll close this for now, I've extracted the new tests to #5654

@MridulS MridulS closed this May 18, 2022
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.

None yet

3 participants