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

Only yield output neighbours in petgraph's IntoNeighbors trait #88

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

aborgna-q
Copy link
Collaborator

No description provided.

@aborgna-q aborgna-q requested a review from acl-cqc June 27, 2023 16:32
Copy link
Contributor

@acl-cqc acl-cqc left a comment

Choose a reason for hiding this comment

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

Yes, but it would be nice to trial this with Hugr first and see that it behaves as I expect; can you run me through how to do that?

@aborgna-q
Copy link
Collaborator Author

I checked it on hugr/main.

If you have a local clone, you can add a path = "../portgraph" attribute to the dependency in Cargo.toml.
Alternatively, you can get it from github

git="https://github.com/CQCL/hugr", branch="fix/petgraph-neighbours"

@acl-cqc
Copy link
Contributor

acl-cqc commented Jun 28, 2023

Hmmmm, I put a panic in here and yet the Hugr code using petgraph DfsPostOrder doesn't panic. Maybe there's another place?

@aborgna-q aborgna-q merged commit bc221b8 into main Jun 28, 2023
@aborgna-q aborgna-q deleted the fix/petgraph-neighbours branch June 28, 2023 10:46
@acl-cqc
Copy link
Contributor

acl-cqc commented Jun 28, 2023

Ok, so turns out this isn't the code we are using in Hugr, so we are at the level of "it looks wrong", but I'll stand by that it does look wrong and this PR looks like a fix and I have approved it :)

@aborgna-q
Copy link
Collaborator Author

It's part of the IntoNeighbors definition:

The neighbors are, depending on the graph’s edge type:

  • Directed: All targets of edges from a.

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