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

fix: Check for rewrite composition in badger #255

Merged
merged 2 commits into from
Nov 21, 2023

Conversation

aborgna-q
Copy link
Collaborator

This is a short-term quick-but-incomplete fix for #239.

We now check right after the composition whether we have invalidated the circuit by making a loop.
We leverage that circuit hashing already does a toposort traversal, adding an error when a loop is found and catching it.

Still, it is theoretically possible for an invalid chain of rewrites to end up producing a valid (but not equivalent) circuit. There is a discussion open for discussing more general solutions to this problem #242.

This is a short-term quick-but-incomplete fix for the rewrite
composition generating loops in the circuit.

We now check right after the composition whether we have invalidated the
circuit.

Still, it is theoretically possible for an invalid chain of rewrites to
end up producing a valid (but not equivalent) circuit.
There is a discussion open for discussing more general solutions to this problem.
@aborgna-q aborgna-q requested a review from cqc-alec November 21, 2023 15:40
@aborgna-q aborgna-q linked an issue Nov 21, 2023 that may be closed by this pull request
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (b077660) 82.92% compared to head (f614659) 82.71%.

❗ Current head f614659 differs from pull request most recent head ef4afd0. Consider uploading reports for the commit ef4afd0 to get more accurate results

Files Patch % Lines
tket2/src/circuit/hash.rs 76.92% 2 Missing and 1 partial ⚠️
tket2/src/optimiser/badger/hugr_pqueue.rs 0.00% 1 Missing ⚠️
tket2/src/optimiser/badger/worker.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
- Coverage   82.92%   82.71%   -0.21%     
==========================================
  Files          34       34              
  Lines        4246     4247       +1     
  Branches     4147     4148       +1     
==========================================
- Hits         3521     3513       -8     
- Misses        537      547      +10     
+ Partials      188      187       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@cqc-alec cqc-alec left a comment

Choose a reason for hiding this comment

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

Clever. Would it be difficult to add a test case to exercise this failure mode (which we'd of course hope to remove in due course)?

@aborgna-q
Copy link
Collaborator Author

I added a test, but left it ignored-by-default since loading the rewrite set takes a while (#257).

@aborgna-q aborgna-q enabled auto-merge November 21, 2023 16:22
@aborgna-q aborgna-q added this pull request to the merge queue Nov 21, 2023
Merged via the queue into main with commit 0b793be Nov 21, 2023
@aborgna-q aborgna-q deleted the feat/check-invalid-rewrite-composition branch November 21, 2023 16:25
ss2165 pushed a commit that referenced this pull request Dec 8, 2023
This is a short-term quick-but-incomplete fix for #239.

We now check right after the composition whether we have invalidated the
circuit by making a loop.
We leverage that circuit hashing already does a toposort traversal,
adding an error when a loop is found and catching it.

Still, it is theoretically possible for an invalid chain of rewrites to
end up producing a valid (but not equivalent) circuit. There is a
discussion open for discussing more general solutions to this problem
#242.
github-merge-queue bot pushed a commit that referenced this pull request May 24, 2024
## 🤖 New release
* `tket2`: 0.1.0-alpha.1

<details><summary><i><b>Changelog</b></i></summary><p>

## `tket2`
<blockquote>

##
[0.0.0-alpha.1](https://github.com/CQCL/tket2/releases/tag/tket2-v0.0.0-alpha.1)
- 2024-05-24

### Bug Fixes
- Check for rewrite composition in badger
([#255](#255))
- induced cycles in depth optimisation
([#264](#264))
- Encode opaque symbolic constants
([#273](#273))
- Correctly detect custom ops by name
([#281](#281))
- Track input linear units in `Command`
([#310](#310))
- Don't convert tket2 measurements into tket1 ops
([#331](#331))

### Documentation
- Expand the main module and README docs
([#298](#298))

### New Features
- add angle type to tket2 extension
([#231](#231))
- bindings for circuit cost and hash
([#252](#252))
- Implement `PyErr` conversion locally in `tket2-py`
([#258](#258))
- Add a "progress timeout" to badger
([#259](#259))
- [**breaking**] Add lexicographic cost
([#270](#270))
- rewrite tracing ([#267](#267))
- Move pre/post rewrite cost to the RewriteStrategy API
([#276](#276))
- [**breaking**] Lexicographic cost fn
([#277](#277))
- Return rewrite strategies as a generator
([#275](#275))
- add qalloc, qfree, reset ops
([#284](#284))
- [**breaking**] Support any ops in portmatching
([#293](#293))
- Add `PatternMatch::nodes` and `subcircuit` + matching example
([#299](#299))
- Use `IncomingPort` and `OutgoingPort` instead of `Port` where
possible. ([#296](#296))
- expose Tk2Op name ([#307](#307))

### Refactor
- Move tket2 code to a workspace member
([#210](#210))
- Restructure the python code
([#211](#211))
- s/taso/badger/ ([#228](#228))
- Move python bindings from `tket2` to `tket2-py`
([#235](#235))
- rename t2op ([#256](#256))

### Testing
- Add small parallel badger test
([#237](#237))
- fix non-deterministic badger test
([#245](#245))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Agustin Borgna <[email protected]>
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.

Cyclic circuit produced during Badger optimisation
2 participants