-
Notifications
You must be signed in to change notification settings - Fork 11
fix: Multiple fixes to the pytket encoder #1226
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1226 +/- ##
==========================================
- Coverage 78.78% 78.71% -0.07%
==========================================
Files 159 160 +1
Lines 20324 20592 +268
Branches 19222 19490 +268
==========================================
+ Hits 16012 16210 +198
- Misses 3332 3392 +60
- Partials 980 990 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
mark-koch
left a comment
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.
Nice! Not an expert on your pytket conversion work, but looks good from the surface 👍
Mostly just some doc nits and questions. Also I'm wondering why this doesn't count as a breaking change due to signature changes/the switch to OpaqueSubgraph?
| /// of subgraphs it can represent; in particular const edges and order edge are | ||
| /// not allowed in the boundary. | ||
| #[derive(Debug, Clone)] | ||
| pub struct OpaqueSubgraph<N> { |
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.
Should we consider upstreaming this? In general, SiblingSubgraph seems quite brittle/not production ready 😅
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.
Yeah, we could have a discussion on which bits of this are useful.
This structure is not specific to the encoder/decoder per-se, but some it's not always useful esp. if the plan is to do rewriting.
There are some plans to redefine the subgraph structures anyways.
| // Ignore nodes with order edges, as they cannot be represented in the pytket circuit. | ||
| if !self.has_order_edges(node, optype, circ) { |
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.
Coming from Guppy, this also includes ancilla allocation and measurement :/
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.
Those would have appeared as input/output qubits to pytket anyways.
We should thing about alternatives in the future, but at least this change makes things correct.
(We'd need to device some way to track relations between pytket commands, which seems hard)
| return true; | ||
| } | ||
|
|
||
| // For now, we just hard-code this to the two kind of bits we support. |
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.
This only works if the sum-bool is used linearly, right? Is this a problem?
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.
Or do we assume that pytket extraction only runs after bool linearisation?
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.
Sum bool is marked as unsupported, so we won't see them here.
tket/src/serialize/pytket/tests.rs
Outdated
| // TODO: fix edge case: non-local edge from an unsupported node inside a nested CircBox | ||
| // to/from the input of the head region being encoded... | ||
| #[should_panic(expected = "Unsupported edge kind")] | ||
| #[case::non_local(circ_non_local(), 1, CircuitRoundtripTestConfig::Default)] | ||
| #[should_panic(expected = "has an unconnected port")] | ||
| #[case::non_local(circ_non_local(), 2, CircuitRoundtripTestConfig::Default)] |
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.
Is this still the same edge case? I don't understand why the message is now "has an unconnected port"?
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.
Thinking about it I'm not sure why it's still failing after some of these fixes 🤔
Let me double check :P
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.
It was a problem with non-local edges from input nodes... It's fixed now :)
This PR is a compilation of fixes to the pytket encoder/decoder. With these implemented, the example from CQCL/hugr#2667 should work correctly.
The fixes include (one per commit):
SiblingSubgraphto define the unsupported subgraphs. That structure is quite opinionated and brittle;QAllocing qubits that don't get consumed afterwards.[float]/[float, rotation].extra_subgraph(that cannot be encoded in the pytket circuit)The main result is a bunch of tests now passing in
tests.rs.