DFS and DFS iterator

[iosonofabio]

This PR aims at addressing #295.

In that issue a trivial Python implementation using recursion is proposed. That would be simplest, of course, but potentially runs into a few issues:

• max depth of the Python call stack
• perhaps speed

The way BFS is coded is via a two-tier system:

• a function that returns a whole list of the traversal, which relies on the C core igraph_i_bfs
• an iterator that reimplements the basic algorithm (a queue)

Both live at the C API interface with Python and are not recursive, therefore should be more performant than an equivalent, pure Python recursive call.

This PR aims at either of the following three:

1. implementing DFS and its iterator the same way that BFS is implemented
2. take down the whole infrastructure for BFS and substitute it with a simple pure-python recursive call and do the same for DFS
3. leave BFS the way it is and use the pure Python recursion for DFS, along the lines of "better slow than nothing"

Any preferences out there among these?

[iosonofabio]

Alright, I implemented two things:

1. Graph.DFSIter at the C/Python interface similar to BFSIter (basically using a stack instead of a queue)
2. Graph.dfs in pure Python but without recursion (also using a stack). This is a temporary implementation until we have a working igraph_i_dfs fuction in C, for which I started igraph_i_dfs igraph#1451. Notice this is not a lazy generator but rather an immediate function: same as Graph.bfs. I suppose the XFSIter are supposed to be the lazy versions.

I also added tests to for these guys and also one test for Graph.bfs which was missing.

There is one thing I am not quite sure about in terms of API. Graph.bfs has these layers, but it seems like the code was written somewhat confusingly. The C/Python interface file seems to think this vector should have length n (number of nodes) and initialized that way, but then the vector gets blissfully cleared inside of igraph_i_bfs and has a variable length - the meaning of which escapes me.

Since I didn't understand it I did not add any layers to DFS. I don't think it's too important as we can use DFSIter to return the distance and parents anyway, but if you have an illuminating explanation I'd love to hear it.

Otherwise ready to merge.

[ntamas]

Thanks a lot for this PR! Yes, the problem with BFS and DFS in general is that the "pythonic" way of doing these is via iterators or generator functions, and neither of them can be connected with the C interface because the C core is fundamentally different (it calculates the whole BFS / DFS in advance). That's why we have .bfs() and BFSIter simultaneously, so the approach you have chosen is correct.

I'll start reviewing this soon.

[ntamas]

Can't we simply use self.neighbors(vid, mode=mode) here? This would avoid creating a Vertex object (the output of self.vs[vid]) and would also simplify the code that comes later when we have to figure out which is the "other" endpoint of the edge.

[ntamas]

Another trick that comes into my mind: we could have another stack where we push the retrieved neighbors of vid when it is visited for the first time. Then, whenever we need to find the next neighbor of vid to visit, we simply pop off the last element of the list. If the list is empty, there are no more neighbors of vid to visit so we can backtrack.

[iosonofabio]

Alright I use neighbors now, good catch.

I tried playing a little with a different stack with all new neighbors, but it didn't make much of a change in terms of speed. I guess we could use a heap to keep the things sorted by distance and parent, but I'm not sure it improves a lot considering it's pure Python anyway.

[ntamas]

Looking good - the only suggestion that should seriously be considered is the usage of self.neighbors(vid) instead of self.vs[vid].out_edges() (or .in_edges()) to retrieve the neighbors. This is a low-hanging fruit that would also simplify some of the code that follows after it.

[iosonofabio]

Good, I made the neighbors and all other changes, ready to merge. If you want to elaborate a bit on the "second stack" idea I'm happy to hear, else let's merge.

[ntamas]

Okay, let's merge this now and I might experiment a bit with the idea that I outlined above.

[ntamas]

FYI, this is what I was thinking about: 5e4a5e2

[iosonofabio]

I see, you're caching the function call, looks good

…

On Tue, Aug 4, 2020, at 21:51, Tamás Nepusz wrote: FYI, this is what I was thinking about: 5e4a5e2 <5e4a5e2> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#315 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJFEABT2MGBYXKVI6V4S43R67Y4NANCNFSM4PLFPIFA>.