-
Notifications
You must be signed in to change notification settings - Fork 20.1k
Add DFS with parent-completion constraint for DAG traversal #6467
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
base: master
Are you sure you want to change the base?
Add DFS with parent-completion constraint for DAG traversal #6467
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6467 +/- ##
============================================
+ Coverage 75.06% 75.11% +0.05%
- Complexity 5540 5575 +35
============================================
Files 685 687 +2
Lines 19219 19318 +99
Branches 3709 3732 +23
============================================
+ Hits 14426 14511 +85
- Misses 4239 4245 +6
- Partials 554 562 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -1,4 +1,4 @@ | |||
/** | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please undo your changes? Or is there any specific reason you did this? @StathisVeinoglou
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I’ve reverted the change as requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that this class name clearly reflects what your implementation does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the spot! I renamed it to better explain what the implementation is for.
return Collections.unmodifiableList(events); | ||
} | ||
|
||
private static <T> void dfs(T u, Map<T, List<T>> succ, Map<T, List<T>> pred, Set<T> visited, int[] order, List<TraversalEvent<T>> out) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it is better to use full names. It will look more descriptive for learning purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea good point! I believe that i replaced all abbreviated names with full descriptive ones.
} | ||
|
||
/** An event emitted by the traversal: either a VISIT with an order, or a SKIP with a note. */ | ||
public static final class TraversalEvent<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be better to use recod here:
/** An event emitted by the traversal: either a VISIT with an order, or a SKIP with a note. */
public record TraversalEvent<T>(
T node,
Integer order, // non-null for visit, null for skip
String note // non-null for skip, null for visit
) {
public TraversalEvent {
Objects.requireNonNull(node);
// order and note can be null based on event type
}
/** A visit event with an increasing order (0,1,2,...) */
public static <T> TraversalEvent<T> visit(T node, int order) {
return new TraversalEvent<>(node, order, null);
}
/** A skip event with an explanatory note (e.g., not all parents visited yet). */
public static <T> TraversalEvent<T> skip(T node, String note) {
return new TraversalEvent<>(node, null, Objects.requireNonNull(note));
}
public boolean isVisit() { return order != null; }
public boolean isSkip() { return order == null; }
@Override
public String toString() {
return isVisit() ? "VISIT(" + node + ", order=" + order + ")"
: "SKIP(" + node + ", " + note + ")";
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I’ve incorporated this exact record definition.
return true; | ||
} | ||
|
||
private static <T> boolean appearsAnywhere(Map<T, List<T>> succ, T node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Seems like it's better to use full descriptive names.
…raightforward about the implementation.Added full names instead of shortcuts, and included record.
Thanks for all the suggestions! I believe that i implemented the changes. |
Description
This PR adds a new
GraphTraversal
class that implements a modified DFS traversal order for directed graphs, ensuring that a node is only visited if all its parents have already been visited.This approach is useful in dependency resolution and neural network computation graph execution order.
clang-format -i --style=file path/to/your/file.java