added iterators for dfs and bfs

[kylebeggs]

trafficstars

Adds support to iterate through the nodes in a graph in a depth-first or breadth-first manner. (see #106 )

[kylebeggs]

The checks are failing because of other parts of the code, not what is included in this PR. I'm still kinda new to this - what is the procedure when this happens?

[simonschoelly]

The checks are failing because of other parts of the code, not what is included in this PR. I'm still kinda new to this - what is the procedure when this happens?

From the error logs, it looks as if in your PR you used some names that where already used for something else:

WARNING: using Traversals.tree in module Main conflicts with an existing identifier.
`` 

[kylebeggs]

Ah, I see. I fixed the issue. Tests still show one error but it seems to be in a completely different module

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.30%. Comparing base (afb8245) to head (10e7c6e). Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
+ Coverage   97.28%   97.30%   +0.02%     
==========================================
  Files         118      120       +2     
  Lines        6876     6944      +68     
==========================================
+ Hits         6689     6757      +68     
  Misses        187      187              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[simonschoelly]

Ah, I see. I fixed the issue. Tests still show one error but it seems to be in a completely different module

Indeed - apparently the tests are failing in Julia v1.8 -> I will need to investigate here

[simonschoelly]

@kylebeggs If you rebase/merge the lastest master in your branch, the tests should pass now.

[kylebeggs]

@simonschoelly @etiennedeg I am taking a new approach to this taking into account some of the comments. I converted this PR to a draft in the meantime

[kylebeggs]

@thchr thanks for the suggestion. @simonschoelly @etiennedeg I am opening this back up as I have made some changes. Please review and lets all work together to come up with something to get us started with iterators in this package.

[Tortar]

This is very cool, one thing I want to suggest for the bfs part is that to apply the optimization I already applied here: https://github.com/JuliaGraphs/Graphs.jl/pull/250.

[kylebeggs]

@Tortar Thanks for the comments. Unfortunately, I don't have time to apply your suggestions right now. Please, make the changes if you are able to. Hopefully one day we can merge this.

[Tortar]

@kylebeggs I'm willing to go on with this, I think we have two options: you can list me as a collaborator of your fork so that I can update this branch, otherwise I will need to superseed this pr. I would prefer the first option so that you appear as the main author

[kylebeggs]

@Tortar sounds great! I'll add you as a collaborator..

[gdalle]

@kylebeggs if we give up on multiple sources for now and focus on a single source, is this in a reviewable / mergeable state?

[Tortar]

Hi @gdalle working on extending to multiple sources at the moment, shouldn't be too hard, sorry for the delay, but I forgot about this

[Tortar]

okay, now multi source versions for bfs and dfs should work, I don't know the Kruskal algorithm so I didn't update it. Also, tests are failing because of jet complaining about the kruskal implementation

[Tortar]

my idea would be to put that algorithm in a separate PR and finish the work for the simpler cases of bfs and dfs, what do you think?

[gdalle]

I agree, let's do BFS and DFS first, they're much more widely used

[Tortar]

I removed the Kruskal implementation, if you can @kylebeggs I think you should open the new PR though since you made it

[kylebeggs]

@Tortar I'm sorry, I'm confused. Why do we need to make a new PR? This one reflects the removal of Kruskal?

[Tortar]

I removed it from here so that we can finish the work for bfs/dfs, so maybe not to forget about the kruskal implementation here: https://github.com/JuliaGraphs/Graphs.jl/pull/163/commits/72c6840dfd75cedc1d1a3286986bd19f3e522b48 you can open up a new pr

[Tortar]

anyway @gdalle it should be ready for review!

[kylebeggs]

@Tortar I see. You have done a lot here, please feel free to make that PR.

[gdalle]

Thank you for the PR! I cleaned up the code a little bit and ensured type stability in the structs

[Tortar]

Thanks @gdalle for the help! But tests are now failing somehow

[gdalle]

Yeah sorry I'll fix it in a moment

[gdalle]

tests are fixed now

[kylebeggs]

Great to see so much progress on this @Tortar @gdalle. What else is needed to get this merged? There has been a lot of activity, so it is a little difficult to see what exactly is left to do?

[gdalle]

Essentially address my unresolved comments above: adding more tests and some comments / docstrings describing the logic to make reviewing easier

[Tortar]

I tried to address all review comments by @gdalle, probably docstrings can be improved but hopefully should be okay nonetheless

[kylebeggs]

Huge thanks to @Tortar !!