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

DOCS DFS cycle detection #96

Merged
merged 43 commits into from
Sep 18, 2023
Merged

DOCS DFS cycle detection #96

merged 43 commits into from
Sep 18, 2023

Conversation

Hromz
Copy link
Contributor

@Hromz Hromz commented Sep 14, 2023

  • Short description
  • Potential uses
  • Limitation
  • Syntax
  • Optional

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Patch has no changes to coverable lines.

📢 Thoughts on this report? Let us know!.

@bobluppes
Copy link
Owner

closes #60

@bobluppes bobluppes self-requested a review September 17, 2023 20:35
@bobluppes bobluppes added the enhancement New feature label Sep 17, 2023
Copy link
Owner

@bobluppes bobluppes left a comment

Choose a reason for hiding this comment

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

Looks very good!
Only one question on which I would value your opinion. Looks good to merge otherwise 👍🏻

Comment on lines 7 to 8

Copy link
Owner

Choose a reason for hiding this comment

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

[minor]: It sounds like this is a disadvantage of the algorithm, but I am wondering if negative weight cycles would ever influence cycle detection. I would argue the algorithm does work for weighted graphs, but the edge weight will simply not be considered 🙈

What do you think about this?

Suggested change
The algorithm doesn't work on edges with weight and therefore cannot be used for negative weight cycles.
The algorithm can be used to detect cycles in the structure of a graph, as it does not consider edge weights.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that your suggestion is more correct and a bit more readable.

Copy link
Owner

@bobluppes bobluppes left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@bobluppes bobluppes merged commit a1e229c into bobluppes:main Sep 18, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants