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 Graph difference #571

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Add Graph difference #571

wants to merge 22 commits into from

Conversation

nahumsa
Copy link
Contributor

@nahumsa nahumsa commented Mar 10, 2022

Related to #440

Adds the difference between two graphs for PyDiGraph and PyGraph.

@coveralls
Copy link

coveralls commented Mar 10, 2022

Pull Request Test Coverage Report for Build 2779975375

  • 62 of 62 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 97.131%

Totals Coverage Status
Change from base Build 2778191284: -0.001%
Covered Lines: 12425
Relevant Lines: 12792

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a 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.

Comment on lines +43 to +46
for node in first.node_indices() {
let weight = &first[node];
final_graph.add_node(weight.clone_ref(py));
}
Copy link
Member

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]?

Copy link
Contributor Author

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
///
Copy link
Member

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.

Copy link
Contributor Author

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.

@georgios-ts
Copy link
Collaborator

Users have only indirect control over node indices since we assign them internally. In union we distinguish two nodes/edges based on their data payloads and it's probably better to do the same in this PR too.
In more detail I'm thinking that we should:

  1. Verify that we can match every node of the first graph into a unique node in the second graph with the same data payload.
  2. Remove an edge e = (u, v) from the first graph if the second graph has an edge with endpoints e' = (m[u], m[v]), where m is the matching found in step (1.), and the edge data payloads agree.

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.

None yet

5 participants