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

added iterators for dfs and bfs #163

Merged
merged 40 commits into from
May 4, 2024
Merged

Conversation

kylebeggs
Copy link
Contributor

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

@kylebeggs
Copy link
Contributor Author

kylebeggs commented Aug 16, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

kylebeggs commented Aug 17, 2022

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

@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@simonschoelly
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Member

@etiennedeg etiennedeg left a 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.

src/traversals/iterators.jl Outdated Show resolved Hide resolved
src/traversals/iterators.jl Outdated Show resolved Hide resolved
@kylebeggs kylebeggs marked this pull request as draft September 19, 2022 13:55
@kylebeggs
Copy link
Contributor Author

kylebeggs commented Sep 19, 2022

@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

src/iterators/bfs.jl Outdated Show resolved Hide resolved
@kylebeggs
Copy link
Contributor Author

@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.

@kylebeggs kylebeggs marked this pull request as ready for review March 15, 2023 13:49
@gdalle gdalle added the enhancement New feature or request label Jun 16, 2023
@Tortar
Copy link
Contributor

Tortar commented Oct 21, 2023

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.

src/iterators/bfs.jl Outdated Show resolved Hide resolved
@gdalle
Copy link
Member

gdalle commented Mar 5, 2024

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

src/iterators/bfs.jl Outdated Show resolved Hide resolved
src/iterators/bfs.jl Show resolved Hide resolved
src/iterators/dfs.jl Show resolved Hide resolved
src/iterators/bfs.jl Show resolved Hide resolved
src/iterators/dfs.jl Show resolved Hide resolved
src/iterators/dfs.jl Show resolved Hide resolved
src/iterators/bfs.jl Outdated Show resolved Hide resolved
src/iterators/dfs.jl Outdated Show resolved Hide resolved
test/iterators/dfs.jl Show resolved Hide resolved
test/iterators/bfs.jl Show resolved Hide resolved
@Tortar
Copy link
Contributor

Tortar commented Mar 5, 2024

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

@gdalle
Copy link
Member

gdalle commented Mar 5, 2024

Yeah sorry I'll fix it in a moment

@gdalle
Copy link
Member

gdalle commented Mar 6, 2024

tests are fixed now

@kylebeggs
Copy link
Contributor Author

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
Copy link
Member

gdalle commented Mar 6, 2024

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

@Tortar
Copy link
Contributor

Tortar commented Apr 28, 2024

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

Copy link
Member

@gdalle gdalle left a 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.

src/iterators/bfs.jl Outdated Show resolved Hide resolved
src/iterators/bfs.jl Outdated Show resolved Hide resolved
src/iterators/dfs.jl Outdated Show resolved Hide resolved
src/iterators/dfs.jl Outdated Show resolved Hide resolved
src/iterators/bfs.jl Outdated Show resolved Hide resolved
@gdalle gdalle merged commit d183c26 into JuliaGraphs:master May 4, 2024
9 of 12 checks passed
@kylebeggs
Copy link
Contributor Author

Huge thanks to @Tortar !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants