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

Add a utility for finding minimal topological sorts #6884

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Conversation

tlively
Copy link
Member

@tlively tlively commented Aug 29, 2024

Reuse the code implementing Kahn's topological sort algorithm with a new
configuration that uses a min-heap to always choose the best available
element.

Also add wrapper utilities that can find topological sorts of graphs
with arbitrary element types, not just indices.

Reuse the code implementing Kahn's topological sort algorithm with a new
configuration that uses a min-heap to always choose the best available
element.

Also add wrapper utilities that can find topological sorts of graphs
with arbitrary element types, not just indices.
@tlively tlively requested a review from kripken August 29, 2024 03:36
@@ -36,7 +38,8 @@ struct TopologicalOrders {

// Takes an adjacency list, where the list for each vertex is a sorted list of
// the indices of its children, which will appear after it in the order.
TopologicalOrders(const std::vector<std::vector<size_t>>& graph);
TopologicalOrders(const std::vector<std::vector<size_t>>& graph)
: TopologicalOrders(graph, false) {}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an enum for this?

const std::vector<size_t>& operator*() const {
return TopologicalOrders::operator*();
}
};
Copy link
Member

Choose a reason for hiding this comment

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

This constructor is identical to the parent, and this operator doesn't seem to modify anything either - I am sure I'm missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, this is providing a small subset of the functionality of the superclass, packaged under a different name that is more appropriate for the common case of needing just one topological sort.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks... I was missing the word "private" on line 108. Makes sense now.

Graph graph(3);
graph[2].push_back(1);
std::vector<size_t> expected{0, 2, 1};
EXPECT_EQ(*MinTopologicalSort(graph), expected);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add comments to these, e.g. for this one IIUC the point is to show that 2 is before 1, and the unconstrained 0 is smaller so it appears before them both.

@tlively tlively merged commit b63aead into main Aug 29, 2024
13 checks passed
@tlively tlively deleted the heap-toposort branch August 29, 2024 22:07
@gkdn gkdn mentioned this pull request Aug 31, 2024
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