simplify stack in dfs

[Tortar]

The stack in dfs can have only parent and children, the depth can be controlled from the outside of it, making the function slightly better in terms of time and space, but also readibility in my opinion :-)

[rossbar]

I personally find it harder to reason about with the depth tracked separately outside of the stack, but that's admittedly pretty subjective. Out of curiosity what's the primary motivation for the proposed change?

[Tortar]

From the speed point of view I measured from a tiny 3% better to a somewhat more relavant 15% better in the best case, so not that much. I think that memory-wise the stack this way consumes probably 1/3 less space, which is good given that it is the second data structure in this method in terms of memory occupied, but all in all, it is not much of a difference, the primary motivation was that I found it interesting to think about this optimization :D

[rossbar]

I think that memory-wise the stack this way consumes probably 1/3 less space

Well, at least 1/3 the number of objects, which is not the same as memory since depth_now is just an integer. I was curious what this would look like in practice I tested with path_graph(10_000_000)¹ and used memray for memory profiling:

On main

This PR

So that's a 400 MB reduction in total usage and 300 fewer allocations for this call (which should help with performance), but accounting for the other changes it ends up being about 300 MB saving for dfs_edges (out of 6.8 GB, so ~4%).

Sorry for the bad screengrab quality - to reproduce (though you may want to use fewer nodes for systems with <16GB ram):

1. Create a script, e.g. dfs_memtest.py

   import networkx as nx
   G = nx.path_graph(10_000_000)
   e = list(nx.dfs_edges(G, source=0))

2. Follow the memray instructions to create the flamegraph

Footnotes

1. The reasoning here was that this graph would maximize the memory saving, as each node only ever has one child, and the nodes are also integers. ↩

[Tortar]

that's interesting, thanks for the flamegraph and the explanation on how to reproduce it! Do you think it is worth it? I don't consider the code less readable, it is actually clearer with this edit at what level we are in the dfs in my opinion

[rossbar]

it is actually clearer with this edit

This is inherently subjective - there is no "right" way to do it. The reason I find it more readable is that the state (i.e. the current depth) is tracked with the current node, whereas the proposed change tracks the state separately. Though again, this is entirely subjective so we're unlikely to make any progress discussing on this front :)

A minor improvement to memory consumption is a more concrete justification for the proposed change. I'd like to hear what others think!

[Tortar]

indeed I was too assertive, I meant to say that it is easier to track when we change levels in the dfs, but anyway I agree that the objective motivation for the change has to be evaluated in relation to the little improvement in memory/time!

[ImHereForTheCookies]

This makes sense to me. 👍

[boothby]

FWIW I agree that a local value is preferable to loading the stack down with longer state tuples. I prefer a slightly different idiom than the one used here:

     while stack:
         parent, children = stack[-1]
         try:
             child = next(children)
             ....
         except StopIteration:
             stack.pop()

# without try/except

    while stack:
        parent, children = stack[-1]
        for child in children:
            ...
            break
        else:
            stack.pop()

In avoiding exceptions, the for,break,else tends to be a little faster.

[boothby]

@Tortar I have implemented my suggestion here: https://github.com/boothby/networkx/tree/tortar-patch-8 -- I would push the changes to your repo but the pre-commit hooks are currently faulty in your branch (not your fault; easily resolved with git rebase -i main) -- after rebasing I cannot push to your branch without --force which is awfully impolite.

Note that we only need to break when we grow the stack -- the for,break,else pattern is most effective in dfs_labeled_edges where non-tree edges are yielded "for free" within the iterator resulting in fewer touches on the stack:

        while stack:
            parent, children = stack[-1]
            for child in children:
                if child in visited:
                    yield parent, child, "nontree"
                else:
                    yield parent, child, "forward"
                    visited.add(child)
                    if depth_now < depth_limit:
                        stack.append((child, iter(G[child])))
                        depth_now += 1
                        break
                    else:
                        yield parent, child, "reverse-depth_limit"
            else:
                stack.pop()
                depth_now -= 1
                if stack:
                    yield stack[-1][0], parent, "reverse"

[Tortar]

thanks @boothby for the improvements! will push them in a moment and make you a co-author of the pull :-)

[dschult]

There is also #6714 which looks to fix #6479...

I think we can/should go ahead and merge these changes as a first step -- and then address the strange behavior seen with depth-limited search.

I'm approving this and I'll have @boothby press the green button. There are a lot of PRs related to this and the order may impact merge conflicts. Best to have control in those cases I guess. :}

Thanks!