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

Change deadlock cycle detection algorithm to a general one for directed graphs #195

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Cay-Zhang
Copy link

@Cay-Zhang Cay-Zhang commented Nov 16, 2022

Description

This PR changes the cycle detection algorithm taught in the deadlock chapter to a generic one for directed graphs. The current algorithm in the textbook has the following problems (possible solutions also outlined):

  • It is said that "the graph is considered as a directed graph" but a cycle detection algorithm that's for undirected graphs is given, which is confusing. We can make it clear that this algorithm does work for RAGs even though they are directed or change the algorithm to a generic one.
  • It detects a cycle in the subgraph reachable from a node instead of in the entire graph. We can make that clear or change the algorithm accordingly.

I would argue that we should change the algorithm taught: there is not much point in teaching a specialized algorithm (it’s general for undirected graphs but RAGs aren’t undirected) and risk confusing students for years. During my office hour, none of the students using the specialized algorithm was aware that it works for RAGs: once I gave them an example of a directed graph that fails the algorithm, they were not able to argue that such a graph is not a valid RAG. And the fact that I gave the examples shows that I wasn’t aware either.

We can still mention the specialized algorithm somewhere as an exercise for capable readers.

Related Issues

N/A

Checklist

  • I updated AUTHORS with my name and email.
  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR builds for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • I updated/added relevant documentation.
  • The title of the PR is descriptive and doesn't have [WIP]
  • I updated CHANGELOG.md to add a description of the change.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does this cause any new material not previously covered in the course to be required?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md).
  • No, this is not a breaking change.

@Cay-Zhang Cay-Zhang marked this pull request as ready for review November 16, 2022 07:47
Copy link
Contributor

@aneeshdurg aneeshdurg left a comment

Choose a reason for hiding this comment

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

Seems reasonable

Have you considered just ripping this part out and replacing it to a link to something with a more detailed explanation of cycle detection as an algorithm? I'm sure there's better resources out there, maybe even with pictures.

return false;
}

bool _is_cyclic(const graph* g, const void* root, set* visited, set* current_path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

_graph_contains_cycle?

Copy link
Author

Choose a reason for hiding this comment

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

Function names are now consistent.

@Cay-Zhang
Copy link
Author

Have you considered just ripping this part out and replacing it to a link to something with a more detailed explanation of cycle detection as an algorithm? I'm sure there's better resources out there, maybe even with pictures.

I failed to find a high-quality explanation that explains the algorithm clearly and provides enough context for a student to be able to easily implement this algorithm in C using the toolchain provided by CS 341. Do you have recommendations @aneeshdurg?

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.

2 participants