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

merge_bbs creating too many copies of self-loop edge #1143

Closed
acl-cqc opened this issue May 31, 2024 · 2 comments · Fixed by #1147
Closed

merge_bbs creating too many copies of self-loop edge #1143

acl-cqc opened this issue May 31, 2024 · 2 comments · Fixed by #1147
Assignees
Labels
bug Something isn't working

Comments

@acl-cqc
Copy link
Contributor

acl-cqc commented May 31, 2024

This relates to #1144 which reveals the bug (by making the Hugr fail validation), previously the multiple-edges had been going unnoticed.

The merge_bbs.rs test in_loop with self_loop: true replaces two BBs with one BB that has an edge back to itself. After performing the Replacement, the new BB in the Hugr has four edges back to itself (all from the same port).
I've checked the Replacement - the replacement Hugr contains exactly one self-edge, mu_out contains exactly one edge leaving the replacement, mu_inp is empty - all as expected.

The problem appears to arise immediately after this:

let InsertionResult { new_root, node_map } = h.insert_hugr(parent, self.replacement);

i.e. before we even inspect mu_inp / mu_out etc. So I believe this is due to CQCL/portgraph#130 but I'm not clear why we see four copies of the edge as from that portgraph issue I would expect to see two.

@acl-cqc acl-cqc added the bug Something isn't working label May 31, 2024
github-merge-queue bot pushed a commit that referenced this issue May 31, 2024
…1144)

This was missed in prior validation, and there are a few instances in
our codebase, so fix them.

I also had to comment out a failing merge_bbs test, see #1143

BREAKING CHANGE: BasicBlocks with multiple successors from the same
outport will now fail to validate (as they should!)
@aborgna-q
Copy link
Collaborator

The next portgraph release should fix that. See CQCL/portgraph#132

@aborgna-q
Copy link
Collaborator

This is now published in portgraph 0.12.1, so we should update it here.

@aborgna-q aborgna-q self-assigned this Jun 3, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 3, 2024
Updates `portgraph` to include the fix in
CQCL/portgraph#132.
Uncomments the previously-failing mentioned in #1143.

Fixes #1143.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants