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 shortest path, critical path also and some affordances to Graph template #1149

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ai-mannamalai
Copy link
Contributor

Add shortest path, critical path also and some affordances to Graph template

  • add shortest path, critical path finding algorithms for Graph; - introduce weighted graph

Copy link
Collaborator

@asraa asraa left a comment

Choose a reason for hiding this comment

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

thanks! could you add a small unit test for the new functions?

@@ -64,6 +74,66 @@ class Graph {
return {};
}

FailureOr<std::vector<V>> get_longest_source_to_sink_path() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit - could you please camelcake the new functions to match the general code style?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's interesting that the formatting checks don't complain about this...

PS: typos + GenAI are a magical combination:
image

@@ -148,10 +218,109 @@ class Graph {
return output;
}

/* get vertices in graph with 0 incoming edges */
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use llvm::copy_if(vertices, [V& v vertex] { return getInEdges(v) == 0; }) if you want!


/* get number of incoming edges into vertex */
uint64_t get_in_degree(V vertex) {
return inEdges.count(vertex) ? 0 : inEdges.at(vertex).size();
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these flipped?

if (result.empty() || result.size() < 3) return result;
std::vector<V> cp_result;
// first node has to be a source node
if (std::find(sources.begin(), sources.end(), result.at(0)) ==
Copy link
Collaborator

Choose a reason for hiding this comment

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

here you can also use the llvm STL helpers (llvm::find(sources, result.at(0))

Copy link
Collaborator

@AlexanderViand-Intel AlexanderViand-Intel left a comment

Choose a reason for hiding this comment

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

Thanks for upstreaming this! I'm unfortunately not really familiar with these utils and what they're used for (thanks @asraa for reviewing!), so a little dummy example/usecase would be super cool (only if you have the time, otherwise it'll just go on the increasingly long pile of "things we should write more documentation for" 😉)

@j2kun
Copy link
Collaborator

j2kun commented Dec 9, 2024

The graph utilities are currently mainly used for our custom circuit vectorizer. We lay out the circuit gates in "levels" according to the dependency graph, and then process the gates in each level as a batch in parallel.

I agree this contribution looks good, but I'd want to know more about what it's planned to be used for, and to see some tests of the new routines to help ensure correctness.

@ai-mannamalai
Copy link
Contributor Author

Fair enough - @AlexanderViand-Intel - I will add some unit tests. Need more time for this.
@asraa - thanks for utility function notes; I can update them as well.

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.

4 participants