-
Notifications
You must be signed in to change notification settings - Fork 145
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 Graph difference #571
base: main
Are you sure you want to change the base?
Add Graph difference #571
Conversation
Rebasing
Rebasing
Pull Request Test Coverage Report for Build 2779975375
💛 - Coveralls |
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.
Overall this LGTM I think this is pretty close, I just have a small question inline about the node weights. The only thing i think that's really missing is in the docstrings for the new function is an explanation of what the difference is, an example might go along way to show what the function does and how it works.
for node in first.node_indices() { | ||
let weight = &first[node]; | ||
final_graph.add_node(weight.clone_ref(py)); | ||
} |
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.
What happens if the node weights are different between first and second? Do we care at all that if first[0] != second[0]
we only preserve the payload for first[0]
?
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.
Do you think we should have a callback to handle weights from both graphs?
|
||
/// Return a new PyGraph that is the difference from two input | ||
/// PyGraph objects | ||
/// |
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.
I think the big thing missing here is the constraints on the input types (they have to have identical node indices). This probably should mention that and also maybe have an example and/or an explanation of how the difference is computed.
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.
Sure! I'll add examples and explain how we compute the difference in the docs.
Users have only indirect control over node indices since we assign them internally. In
|
Related to #440
Adds the difference between two graphs for
PyDiGraph
andPyGraph
.