-
Notifications
You must be signed in to change notification settings - Fork 93
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
added iterators for dfs and bfs #163
Conversation
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:
|
Ah, I see. I fixed the issue. Tests still show one error but it seems to be in a completely different module |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 ☔ View full report in Codecov by Sentry. |
Indeed - apparently the tests are failing in Julia v1.8 -> I will need to investigate here |
@kylebeggs If you rebase/merge the lastest master in your branch, the tests should pass now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your work on this, very appreciated.
@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 |
@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. |
This is very cool, one thing I want to suggest for the bfs part is that to apply the optimization I already applied here: #250. |
Thank you for the PR! I cleaned up the code a little bit and ensured type stability in the structs |
Thanks @gdalle for the help! But tests are now failing somehow |
Yeah sorry I'll fix it in a moment |
tests are fixed now |
Essentially address my unresolved comments above: adding more tests and some comments / docstrings describing the logic to make reviewing easier |
I tried to address all review comments by @gdalle, probably docstrings can be improved but hopefully should be okay nonetheless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution!
There are things we could improve, like using the eltype
of the graph instead of Int
or testing on directed graphs, but this has been cooking for far too long and it is a needed feature.
Huge thanks to @Tortar !! |
Adds support to iterate through the nodes in a graph in a depth-first or breadth-first manner. (see #106 )