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.

Sign up for GitHub

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

Jump to bottom

Correct cycle finding for graphs #2199

Merged
merged 5 commits into from
Oct 27, 2024
Merged

Correct cycle finding for graphs #2199

merged 5 commits into from
Oct 27, 2024

Conversation

byorgey
Copy link
Member

@byorgey byorgey commented Oct 21, 2024

Closes #2198 . Adds a new function findCycle for finding directed cycles in graphs via DFS, and updates failOnCyclicGraph to use it rather than SCCs for reporting cycles.

@byorgey byorgey requested a review from kostmo October 21, 2024 21:48
@byorgey
Copy link
Member Author

byorgey commented Oct 26, 2024

@kostmo any thoughts on this?

kostmo
kostmo approved these changes Oct 26, 2024
View reviewed changes
Copy link
Member

@kostmo kostmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix. Surprised there's not some more off-the-shelf solution though.

@byorgey
Copy link
Member Author

byorgey commented Oct 27, 2024

Surprised there's not some more off-the-shelf solution though.

Well, Data.Graph from containers is somewhat impoverished. I was going to say that we could use a more off-the-shelf solution if we switched to fgl... but actually, I don't see any cycle-finding algorithms there either.

See here for a related discussion: haskell/containers#978

@byorgey byorgey added the merge me Trigger the merge process of the Pull request. label Oct 27, 2024
@mergify mergify bot merged commit 39adce6 into main Oct 27, 2024
14 checks passed
@mergify mergify bot deleted the cycle-finding branch October 27, 2024 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kostmo kostmo kostmo approved these changes

Assignees
No one assigned
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cycles are reported incorrectly
2 participants
@byorgey @kostmo