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

All simple paths (refresh #20) #353

Merged
merged 16 commits into from
Apr 5, 2024
Merged

Conversation

thchr
Copy link
Contributor

@thchr thchr commented Mar 18, 2024

This refreshes #20 by @etiennedeg.
(I couldn't figure out how to do it there directly and also didn't want to bother with fixing merge conflicts either)

The original implementation is by @i-aki-y in sbromberger/LightGraphs.jl#1540.

This "refresh" also simplifies the original implementation a bit (to the extent that I understand it), implements the comments/suggestions that were made in #20 (e.g., cutoff is now set to nv(g)), and improves the documentation (and removes superfluous documentation of internal methods), and removes redundant methods (e.g., the length and collect implementations; the latter comes from Base, and the former is a performance trap).

Cc. @gdalle cf. Slack conversation about this (and him pointing out the existence of #20).

I suspect there will be some issue from Formatter.jl: but there's no clue as to what's wrong when I run it locally, just that something is wrong.

thchr and others added 2 commits March 18, 2024 17:23
- this updates the port of sbromberger/LightGraphs.jl#1540 from JuliaGraphs#20
- has a number of simplifications relative to original implementation
- original implementation by @i-aki-y
- cutoff now defaults to `nv(g)`

Co-authored-by: akiyuki ishikawa <aki.y.ishikawa@gmail.com>
Co-authored-by: Etienne dg <etienne.mace_de_gastines@insa-rouen.fr>
@thchr thchr changed the title All simple paths All simple paths (refresh #20) Mar 18, 2024
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 95.16129% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 97.28%. Comparing base (e773bce) to head (1c9a813).
Report is 5 commits behind head on master.

Files Patch % Lines
src/traversals/all_simple_paths.jl 95.16% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
- Coverage   97.29%   97.28%   -0.02%     
==========================================
  Files         117      118       +1     
  Lines        6814     6876      +62     
==========================================
+ Hits         6630     6689      +59     
- Misses        184      187       +3     

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

@gdalle
Copy link
Member

gdalle commented Mar 18, 2024

documentation seems to be failing

@thchr
Copy link
Contributor Author

thchr commented Mar 18, 2024

documentation seems to be failing

Should be fixed now, thanks.

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.

I have reviewed everything except the actual DFS, because I'm wondering if there is an exploitable overlap with #163

src/traversals/all_simple_paths.jl Show resolved Hide resolved
src/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
test/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
test/traversals/all_simple_paths.jl Show resolved Hide resolved
test/traversals/all_simple_paths.jl Show resolved Hide resolved
src/traversals/all_simple_paths.jl Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
src/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
src/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
src/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
@thchr
Copy link
Contributor Author

thchr commented Mar 22, 2024

There is one API question we should settle: what should the set of all paths from u to v = u be? I.e., what happens when the start and end point are identical? To my mind, it should return [[u]] which is a path of zero length. Currently, however, it returns no paths, i.e., [Int[]].

I propose we change it to return [[u]] in this case.

EDIT: to be more explicit, the choices are between the following behaviors.

Current behavior:

g = path_graph(4)
@test Set(all_simple_paths(g, 1, 1)) == Set(Int[])
@test Set(all_simple_paths(g, 1, [1, 4])) == Set([[1, 2, 3, 4]])

Suggested behavior:

g = path_graph(4)
@test Set(all_simple_paths(g, 1, 1)) == Set([[1]])
@test Set(all_simple_paths(g, 1, [1, 4])) == Set([[1], [1, 2, 3, 4]])

Although, honestly, the suggested behavior is not so natural to implement in the current iterator scheme, so I'd be happy to keep as-is also.

@thchr
Copy link
Contributor Author

thchr commented Mar 22, 2024

I've gone ahead an implemented the proposed u = v special-casing, adding corresponding tests also.

@gdalle
Copy link
Member

gdalle commented Apr 3, 2024

I agree with your suggestion that paths from u to u should be [u] and not [].
This is also more coherent with the way lengths are measured in the algorithm: a path [u] has length 0

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.

I'm now reasonably confident that the implementation is correct! A few minor tweaks and I can merge

src/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
src/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
src/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
src/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
src/traversals/all_simple_paths.jl Show resolved Hide resolved
src/traversals/all_simple_paths.jl Show resolved Hide resolved
src/traversals/all_simple_paths.jl Show resolved Hide resolved
test/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
test/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
test/traversals/all_simple_paths.jl Outdated Show resolved Hide resolved
@gdalle gdalle linked an issue Apr 3, 2024 that may be closed by this pull request
thchr and others added 6 commits April 5, 2024 17:03
Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
@thchr
Copy link
Contributor Author

thchr commented Apr 5, 2024

Thanks for the second review. I've updated everything accordingly, with the exception of any action on your _stepback! question: I simply don't know (and admit that I do not have time or motivation to find out at the moment; I just want something that works and doesn't stall again).

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!

@gdalle gdalle merged commit c1cedee into JuliaGraphs:master Apr 5, 2024
7 of 12 checks passed
@thchr thchr deleted the all-simple-paths branch April 5, 2024 16:02
@mschauer
Copy link
Contributor

mschauer commented Apr 5, 2024

Sorry I only see this now that the work is done. Anyway thank you!

@mschauer
Copy link
Contributor

mschauer commented Apr 5, 2024

Cc @mwien Graphs.jl has now path enumeration

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

Successfully merging this pull request may close these issues.

[Port] A function that finds all simple paths
3 participants