Repository navigation
feat: Add implement_ops and encode pass to the toy_k2 architecture - #407
PabloAndresCQ wants to merge 27 commits into
Conversation
hsemenenko
left a comment
There was a problem hiding this comment.
Adding a few comments.
| @guppy | ||
| @no_type_check | ||
| def is_borrowed(self) -> bool: | ||
| """Check if any address in the block is borrowed.""" | ||
| return self.borrowed_addr[0] or self.borrowed_addr[1] |
There was a problem hiding this comment.
Can't we use the existing Guppy .is_borrowed method on the array?
There was a problem hiding this comment.
That's an interesting idea. I could also replace the borrow_addr and restore_addr by take and put.
There was a problem hiding this comment.
After giving it a try, I'm not convinced this is worth doing. IMO it makes the code slightly harder to understand since you need to be aware of what put/take does on Guppy. What are there any benefits to doing this?
There was a problem hiding this comment.
Very much butting in here but I'm not convinced by this logic - it seems like you are saying, "You'd need to be aware of what the library does, so instead I'll repeat the library here"
There was a problem hiding this comment.
A bigger obstacle (although perhaps only for the bigger change of put_block/take_block to use an array without Option) is that we need empty_array to initialize a STATE, from Henry's PR (but not in guppy 1.1.x)
There was a problem hiding this comment.
Actually I'm not sure we can use borrow-array's is_borrowed here (on a LogicalBlock) - that would mean taking the qubit out from the block, but we don't actually want to do that here.
There was a problem hiding this comment.
Yes, I think that using Guppy's is_borrowed actually doesn't fit here. What we're checking is whether a logical qubit has been borrowed from a block, but the LogicalBlock doesn't have an array of logical qubits, it has an array of physical qubits.
| # Remove the remaining addresses of this block from the stack | ||
| # NOTE: Ideally, `avail_dyn_addrs` would be a data | ||
| # structure with fast deletion (e.g. a set). | ||
| # Generally, this stack will be small, though, so it | ||
| # shouldn't be a performance bottleneck. | ||
| aux_stack: Stack[int, comptime(n_blocks)] = empty_stack() | ||
| for _ in range(len(self.avail_dyn_addrs)): | ||
| addr = self.avail_dyn_addrs.pop() | ||
| if addr[0] != blk_id: | ||
| aux_stack.push(addr) | ||
| for addr in aux_stack: | ||
| self.avail_dyn_addrs.push(addr) |
There was a problem hiding this comment.
Could we instead have a counter on each block to indicate how many "live" qubits there are? Once it reaches 0, all addresses are pushed back to the Stack.
There was a problem hiding this comment.
The current approach is for avail_dyn_addrs to be:
avail_dyn_addrs: Stack of available addresses on existing dynamic blocks.
So your suggestion "Once it reaches 0, all addresses are pushed back to the Stack." would change the semantics.
If you do what you suggest, a block that has been assigned once as dynamic is always used for dynamic qubits. The reason why avail_dyn_addrs includes only addresses on existing dynamic blocks is so that, once a dynamic block is fully freed, it may be used in "user/compiler-handled addresses mode".
|
|
||
| @guppy | ||
| @no_type_check | ||
| def allocate_block(self, logical_block: LogicalBlock[4] @ owned) -> int: |
There was a problem hiding this comment.
This function doesn't allocation the block, so perhaps allocate_block_addr is more appropriate.
There was a problem hiding this comment.
I don't want to use _addr because I want to reserve that to qubit addresses. I'll use allocate_block_id.
| @guppy | ||
| @no_type_check | ||
| @link_name("guppyft.toy_k2._h_dynq") | ||
| def _h_dynq(addr: tuple[int, int]) -> tuple[int, int]: |
There was a problem hiding this comment.
If h_dynq needs an ancilla block, shouldn't we instead treat it like t-state injection where the op accepts a logical block?
There was a problem hiding this comment.
Two things to consider:
- Us giving
inject_tthe magic state as input is at the level of logical primitive. This is because we want to be able to link an opaqueprep_t_statewith a state factory.get_state()atimplement_ops. This does not quite apply here, becauseh_dynqis a non-primitive, and we build it by calling_prep_zero_plus_ft, which would already call a state factory. - We don't need to call RUS state factories in this case, because
_prep_zero_plus_ftis deterministic (note: this was not the case when you reviewed; since I used a zero state prep).
The first bullet point is yet another reason why it'd be best not to have h_dynq as a HUGR op. I'm fine with Guppy "helpers" in logical.py silently allocating qubits (we could add @expected_qubits annotations). I'm not fine with this crucial detail being hidden within an opaque op, only to have the semantics be introduced at the implement_ops level.
65a5283 to
ce788c1
Compare
ce788c1 to
ec3ef07
Compare
hsemenenko
left a comment
There was a problem hiding this comment.
Looks good. I've added some comments. Let's chat about the dynq callback op.
| # Measure the whole block because that's more efficient | ||
| m0, m1 = _measure_z_all(y_blk_id) | ||
| # But only pick the measurement outcome we care about for injection | ||
| m = m0 if addr[1] == 0 else m1 |
There was a problem hiding this comment.
Do we only care about m0 here as we always inject the 0th state?
There was a problem hiding this comment.
Huh. Yeah, that's a bug. Either the cx_dynq in this implementation should target (y_blk_id, addr[1]) or the measurement should always use m0. The implementation in logical.py uses the former. That's an odd mistake to make when copy-pasting... I'll have a look through the _dynq implementations again, as well as logical.py to check they match.
Note: same issue in t_dynq.
There was a problem hiding this comment.
Fixed -> f8fdb6a.
I've also looked through the rest of the dynq ops and compare to the logical addressable implementations. Looks fine.
| # Measure the whole block because that's more efficient | ||
| m0, m1 = _measure_z_all(t_blk_id) | ||
| # But only pick the measurement outcome we care about for injection | ||
| m = m0 if addr[1] == 0 else m1 |
| for shot in res: | ||
| assert shot.keys() == {"x", "z", "s", "sdg"} | ||
| assert all(v for v in shot.values()) |
There was a problem hiding this comment.
I think checking equality with a dictionary would be better. Similarly with other tests that use all(...).
| for shot in res: | |
| assert shot.keys() == {"x", "z", "s", "sdg"} | |
| assert all(v for v in shot.values()) | |
| for shot in res: | |
| assert shot == { | |
| "x": [1], | |
| "z": [1], | |
| "s": [1], | |
| "sdg": [1], | |
| } |
There was a problem hiding this comment.
Now that you write it as a dictionary, I realise the current all(v for v in shot.values()) is not actually testing what I intended, because Python will say that [1] is True (so would [0]) rather than tell me it's not a bool... :old-man-yells-at-python:.
I'll use dictionaries as you suggest for the sake of being explicit.
There was a problem hiding this comment.
Turns out that there were some errors in the tests that were not being found due to [0] being interpreted as True. Fixes here -> cf871cf
|
|
||
| return wrapper # type: ignore[no-any-return] | ||
|
|
||
| lib = GuppyLibrary.from_members( |
There was a problem hiding this comment.
Steane has added some methods to simplify this boilerplate that should be used here.
There was a problem hiding this comment.
Very cool. I've updated it.
0f73856 to
f8fdb6a
Compare
… use tuple instead of array
…e not being found previously
520127a to
9387c3e
Compare
…R op rather than a compound op
PabloAndresCQ
left a comment
There was a problem hiding this comment.
Relevant changes listed below
- Merged #497
There was a problem hiding this comment.
Due to _dynq ops being dropped from the logical API (see latest changes in #326), we can no longer have measure_z_dynq as a compound op of logicals (it used to compose the bindings of project_z_dynq and free_dynq. So, I'm adding it as a new extension op, since we need to compile tket2.ops.MeasureFree to it.
| @dataclass(frozen=True, kw_only=True) | ||
| class _ToyK2Options: | ||
| qed_policy: QEDPolicy |
There was a problem hiding this comment.
Previously this used EncoderParams which was removed in #465. I'm following the pattern introduced in that PR for the options of the architecture, but I have not added the annotation part of that PR.
Let me know if I should. It looked like it was not essential for ToyK2, since it's not meant to be integrated with other components.
| def prep_zero_ft_rus() -> LogicalBlock[4]: | ||
| r"""Fault-tolerant preparation of a logical :math:`|00\\rangle` state. | ||
|
|
||
| Unlike `prep_zero_ft`, this function will keep attempting the preparation | ||
| until it succeeds. |
There was a problem hiding this comment.
I just noticed that the implement_ops of ToyK2 was assuming prep_zero_ft succeeded on the first attempt. My original intention was that it'd do RUS, which is why I added this primitive here (which is now used by implement_ops).
Sure, factories would be better for performance. But performance is not the goal for ToyK2, and we already have Steane for an educational example on how to implement state factories, we don't need to duplicate it here.
Closes #243
NOTE: There's duplication of the non-primitives dynamic ops. See the comment highlighted here: https://github.com/Quantinuum/guppyft/pull/407/changes#r4029285636. @hsemenenko has a draft PR #429 which illustrates a promising direction to avoid this.