feat: Add new tket-qec crate with Iceberg code extension.#1416
feat: Add new tket-qec crate with Iceberg code extension.#1416
Conversation
|
A new Rust package was added to the workspace and is not present in the baseline. cargo-semver-checks output |
tket-qec/src/iceberg/ops.rs
Outdated
| MeasureSyndrome, | ||
| /// Destructive measurement of all qubits. | ||
| MeasureAll, | ||
| /// Non-destructive measurement of one qubit in the X basis. | ||
| MeasureOneX, | ||
| /// Non-destructive measurement of one qubit in the Z basis. | ||
| MeasureOneZ, |
There was a problem hiding this comment.
We could also add lazy versions of these, that return future bools. But I think I prefer to keep it like this in the logical extension; the lowering to physical can convert to future bools if needed.
There was a problem hiding this comment.
This has proved very annoying. I would prefer to use future bools here, and lowering to phyiscal can convert them to bools if needed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1416 +/- ##
==========================================
+ Coverage 79.60% 80.17% +0.57%
==========================================
Files 155 157 +2
Lines 20335 21035 +700
Branches 19345 20045 +700
==========================================
+ Hits 16187 16865 +678
- Misses 3188 3211 +23
+ Partials 960 959 -1
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:
|
PabloAndresCQ
left a comment
There was a problem hiding this comment.
the follow-up to this is compiling from computational to logical and from logical to physical
Unfortuntaly, I doubt we will be able to have the lowering to physical in a public repository. Physical circuits are being considered secret sauce atm. This may have implications on where the extension should live too. IMO this being in the public repository is fine, since we'll (probably?) want to have a public interface to the code library, but it's another factor to consider when deciding whether to add this into tket2 or its own repo.
The PR looks good to me. I'm not accepting just because I'd like someone else that knows about extensions to have a quick look, many of the details went over my head. Otherwise, the gateset and new type look good.
doug-q
left a comment
There was a problem hiding this comment.
My preference is to be in the same repo, but I like a new crate. I like how it will force us to provide better support out-of-tket extensions
| /// Logical Iceberg operations. | ||
| #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, EnumIter, IntoStaticStr, EnumString)] | ||
| #[non_exhaustive] | ||
| pub enum IcebergOpDef { |
There was a problem hiding this comment.
I prefer lower case for these names, as they become the op names.
tket-qec/src/iceberg/types.rs
Outdated
| } | ||
|
|
||
| /// Get a usize from a [`TypeArg`]. | ||
| pub fn get_usize(arg: &TypeArg) -> Result<usize, TermTypeError> { |
There was a problem hiding this comment.
This fails when an arg is a Variable use.
I'm not sure yet whether we are able to accept variables.
This should be arg.as_usize or arg.as_nat, not sure if it exists.
There was a problem hiding this comment.
- https://docs.rs/hugr/latest/hugr/types/type.TypeArg.html#method.as_nat
- We should not reject variables as the code does now.
- We can trust that a type arg corresponding to a type param is legal. So our validation logic should be "try and get the arg values. If this succeeds (because it's not a variable use), check it is a valid value. At some point in the pipeline everything is monomorphised, and invalid variable substitutions will be caught.
tket-qec/src/iceberg/ops.rs
Outdated
| use super::types::{block_tv, get_usize}; | ||
|
|
||
| /// The extension identifier. | ||
| pub const EXTENSION_ID: ExtensionId = ExtensionId::new_unchecked("logical.iceberg.ops"); |
There was a problem hiding this comment.
I prefer to include tket. prefix, especially if it lives in this repo
tket-qec/src/iceberg/ops.rs
Outdated
| ArgsValidator { n_idx: 0 }, | ||
| ) | ||
| .into(), | ||
| AllocZero => CustomValidator::new( |
There was a problem hiding this comment.
I think it would be good to include an alternative constructor, that takes the qubits explicitly, rather than allocating them internally.
If you included this you should obviously also include a destructor to recover the qubits.
I like this to keep our options open in the future if the "internal qubit allocation" that this (i.e. AllocZero) interface requires is annoying.
There was a problem hiding this comment.
I'm unsure about this. Ideally programs would not mix physical and logical qubits, as this blurs the responsibility for tracking what's allocated to what. I suggest we keep to this one for now and see if it becomes a problem.
There was a problem hiding this comment.
We were using what Doug suggests for our auto-encoding rewrite, but we are removing this now.
- We were doing this so that the runtime qubit allocator within QCorrect had all physical qubits reserved in advance (not in a logical state) and QAlloc caused it to prepare them in a logical state (see current main branch).
- Instead, in this PR, Henry changed it so that the runtime allocator has
Option[Iceberg[k]].
I prefer the latter approach (same as what Alec did), so that a user of this extension (including our auto-encoding pass) can only think of allocation and lifetime of logical blocks. This is for safety reasons: I don't want users to be able to pretend that a bunch of physical qubits is a logical block before it has gone through AllocZero or another preparation circuit (i.e. before it has been put into a state in the codespace).
I could be convinced to have something like Doug proposes if we had a different type DirtyBlock that AllocZero consumes to produce an IcebergBlock. That'd be similar to the notion of Money from BRAT, where you reserve the resources for a Qubit, but it's not a valid type to give to quantum operations. I can see the potential of something like this in the long run, but until we see the need for Money in Guppy, I think it's premature to add it here.
@hsemenenko keen to hear your thoughts on this. You mighty have a more pragmatic perspective on why what Doug proposes could be useful in the near future.
There was a problem hiding this comment.
I can't think of a situation in which it makes sense for AllocZero to take physical qubits as input. As Alec said, it seems strange to be mixing physical and logical here and it wouldn't make sense for it to take IcebergBlock as input. DirtyBlock would make sense but I don't think it's something that should be included in the extension.
I suppose having qubits as input allows more control over qubit allocation into blocks, which could be important in large scale systems, but I don't think that's what we want in this extension either.
However, does it make sense to have a ResetBlock which takes IcebergBlock and returns it to the all zero state?
| Arc::downgrade(&EXTENSION) | ||
| } | ||
|
|
||
| fn init_signature(&self, _extension_ref: &Weak<Extension>) -> SignatureFunc { |
There was a problem hiding this comment.
This interface does not allow dynamic indexing in any ops. Is this intentional and Ok??
There was a problem hiding this comment.
That is a very good point which I hadn't considered. I'm not at all sure that it is OK.
There was a problem hiding this comment.
@PabloAndresCQ What is your opinion on this? I'm on the fence. Making all the addressable ops accept dynamic addresses would allow more programmer flexibility (e.g. "apply CX to the qubits in this block indexed by my variables i and j") -- but would also make these ops fallible (unless we say that they are silent no-ops when the index is out of bounds or if the indices of a two-qubit operation are the same, which seems like a footgun). I'm also not sure what the implications are for lowering.
There was a problem hiding this comment.
Since this is not an extension that standard users would interact with, but just (1) QEC experts and (2) developers of encoding and optimisation passes... I think sacrificing safety for the sake of flexibility is justified.
In terms of lowering, I'm not sure I fully understand the implications, but my gut feeling is that it could be problematic. Since Guppy users are allowed to do things like:
for i in range(my_size):
cx(qs[i], other_qs[i])
this would be lowered to a call to logical CX where each iteration is on different qubits. Would your current proposal need us to unroll the loop to be able to know the addresses where we do the CX gates ahead of time?
Relatedly, does this have implications to runtime allocated logical qubits? Even though I'm not fully convinced by the approach, I wouldn't want to shut it down. I expect we'll need to do something hybrid, so we'd need full flexibility, at least when prototyping.
There was a problem hiding this comment.
Thanks Pablo. I think I'm convinced we should allow dynamic addressing, since otherwise I don't see how we could avoid having to unroll loops like that.
I'm also not convinced of the runtime-allocation approach. I guess having dynamic addressing could make it more difficult for some programs, but I don't have a clear idea of when and how much.
There was a problem hiding this comment.
It could be that we actually want both, since they both have advantages. The advantage of static indices (as well as safety guarantees) is that it is easier to reason about them, during compilation and lowering.
| @@ -0,0 +1,699 @@ | |||
| //! Extension providing logical operations on Iceberg codeblocks. | |||
There was a problem hiding this comment.
I would like some high level documentation about what types and ops are provided, and what their type args mean.
tket-qec/src/iceberg/ops.rs
Outdated
| /// X gate on one qubit with Z on all others. | ||
| XWithAllButOneZ, | ||
| /// Z gate on one qubit with X on all others. | ||
| ZWithAllButOneX, |
There was a problem hiding this comment.
IMO all of the gates from XX up to ZWithAllButOneX should not be in the extension. Any product of Paulis is trivially implemented by the sequence of their single qubit logical components. Naively, one may think that it is important to have these variants for performance, but:
- Single qubit squashing will remove any inefficiency.
- We want to do Paulis on SW anyway (Pauli frame tracking).
This does not apply to rotation gates; all the variants of those are useful because, at the physical level, they are not single qubit gates.
There was a problem hiding this comment.
Happy to remove them, but just for my understanding:
- How does single-qubit squashing remove the inefficiency? If we have a bunch of individual X gates (that could be more efficiently implemented with a global X), they won't be removed by squashing will they? Or do you mean that after lowering to the physical level and then squashing single-qubit gates the two cases will end up being equivalent?
- If we are always going to be doing Paulis in software then do we even need the single-qubit variants?
There was a problem hiding this comment.
- If we have a bunch of individual X gates (that could be more efficiently implemented with a global X), they won't be removed by squashing will they?
Ah yes, my bad. AllX is using that IXXXXI ~ XIIIIX by stabiliser multiplication. SQ squashing cannot remove that inefficiency.
- If we are always going to be doing Paulis in software then do we even need the single-qubit variants?
Yes, because the user might want to issue X or Z gates in their program.
There was a problem hiding this comment.
To clarify: the X and Z gates they issue would simply update the Pauli frame, without applying anything at the physical level.
One thing to add, though, is that if we go with Pauli frame tracking (I'm sure we will), it's also useful to have an instruction ResolveFrame that forces the Paulis tracked in the frame to be applied at the physical level. In principle, this isn't needed, but it's reasonable to expect users would want to force their application (and have the Pauli frame restored to all I).
There was a problem hiding this comment.
OK, I will remove those ops. Should I add ResolveFrame to this extension? Presumably this would be useful for a wide variety of codes so maybe belongs in a separate extension.
There was a problem hiding this comment.
Yeah, ResolveFrame would belong to a separate generic extension. Maybe best to leave it out from here.
There was a problem hiding this comment.
Better not to remove them yet. I am questioning if it makes sense to have Pauli frame tracking on Iceberg. The use case of applying QEC corrections in SW doesn't apply on a d=2 code.
I've implemented a prototype of Pauli frame tracking and I'm not convinced about it. I'll make a draft PR to gather feedback.
There was a problem hiding this comment.
OK, will put them back.
There was a problem hiding this comment.
Ah apologies, I thought they had not been removed yet (at least they do show up in GitHub).
There was a problem hiding this comment.
Yeah I've been making some updates locally, but no problem to revert that.
This reverts commit a05d402.
| /// Extension version. | ||
| pub const VERSION: semver::Version = semver::Version::new(0, 1, 0); | ||
|
|
||
| /// Logical Iceberg operations. |
There was a problem hiding this comment.
I like this, thank you.
-
I think there must be a canonical paper defining all these ops, could you link to it?
-
You could add that we expect frontends to generate dynamic ops and to transform those, where possible, into static ops.
There was a problem hiding this comment.
I think there must be a canonical paper defining all these ops, could you link to it?
Not really, unfortunately. They're spread in multiple papers, and some probably don't show up in any. A couple of references of relevant papers (but not exhaustive):
| use IcebergOpDef::*; | ||
| match self { | ||
| x => "apply an X gate to one qubit", | ||
| x_d => "apply an X gate to one qubit (with dynamic index)", |
There was a problem hiding this comment.
I think this would be better without the parenthesis.
| if n != 1 + self.n_idx { | ||
| return Err(SignatureError::InvalidTypeArgs); | ||
| } | ||
| let k = arg_values[0].as_nat().unwrap(); |
There was a problem hiding this comment.
This will panic when a variable is used as a type arg. We should either fail (because a variable is invalid here) or continue. I prefer the latter.
let mb_k = arg_values[0].as_nat();
if let Some(k) = mb_k {
...
}
We should ensure there are tests for instantiations with variables(I haven't checked yet if there are)
There was a problem hiding this comment.
I agree we should return an error instead of panic if the TypeArg is invalid, and add tests for that case. But I don't understand what you mean by "variable" here. I don't think you mean we should allow k to be dynamic (and I don't think we should). Could you give an example of what you think should be allowed that is not?
There was a problem hiding this comment.
I claim we should allow a type arg to be a variable use of a param bound in a function, allowing one to write a function generic over the size of the code e.g.
foo[N: usize]: [block[N]] -> [[block[N]]
here the type of the inputs and outputs would contain a use of the N variable, and any iceberg ops inside the function would also need to use the N variable. I hope this makes sense?
There was a problem hiding this comment.
When defining the Guppy library for the code, we definitely want to be able to define functions generically over the block size, for instance:
k = guppy.nat_var("k")
@guppy
def logical_global_h(ice: Iceberg[k]) -> None:
h(ice.top)
for i in range(k):
h(ice.block[i])
h(ice.bottom)
mem_swap(ice.top, ice.bottom)
However, what I had in mind is that the value of k will be known at compile time, so when we are producing the HUGR, we don't need to treat k as a variable. To me that sounds sufficient for the purpose of optimisation (and it's simpler so probably helpful). What isn't clear to me is whether that'd make the compilation to the HUGR using this extension harder... but it shouldn't in our current approach, since the user needs to specify the number of logical qubits per block when calling the auto-encoding pass.
| if arg_values.len() != 3 { | ||
| return Err(SignatureError::InvalidTypeArgs); | ||
| } | ||
| let k = arg_values[0].as_nat().unwrap(); |
There was a problem hiding this comment.
panics here also need fixing
| pub k: u64, | ||
|
|
||
| /// Qubit index parameters. | ||
| pub indices: Vec<u64>, |
There was a problem hiding this comment.
If we are allowing variable uses in type args, then these u64s need to be TypeArgs
PabloAndresCQ
left a comment
There was a problem hiding this comment.
Looks good to me! Looking forward to using this extension in the auto-encoding pass.
I think the major changes that me/Doug suggested have been addressed (except the one on AllocZero receiving an input dirty block, which I don't agree with). I'm accepting on my end, but it'd be good to wait for Doug to confirm too. Also, probably worth for @hsemenenko to have at least a skim through this in case anything stands out.
| if n != 1 + self.n_idx { | ||
| return Err(SignatureError::InvalidTypeArgs); | ||
| } | ||
| let k = arg_values[0].as_nat().unwrap(); |
There was a problem hiding this comment.
When defining the Guppy library for the code, we definitely want to be able to define functions generically over the block size, for instance:
k = guppy.nat_var("k")
@guppy
def logical_global_h(ice: Iceberg[k]) -> None:
h(ice.top)
for i in range(k):
h(ice.block[i])
h(ice.bottom)
mem_swap(ice.top, ice.bottom)
However, what I had in mind is that the value of k will be known at compile time, so when we are producing the HUGR, we don't need to treat k as a variable. To me that sounds sufficient for the purpose of optimisation (and it's simpler so probably helpful). What isn't clear to me is whether that'd make the compilation to the HUGR using this extension harder... but it shouldn't in our current approach, since the user needs to specify the number of logical qubits per block when calling the auto-encoding pass.
tket-qec/src/iceberg/ops.rs
Outdated
| ArgsValidator { n_idx: 0 }, | ||
| ) | ||
| .into(), | ||
| AllocZero => CustomValidator::new( |
There was a problem hiding this comment.
We were using what Doug suggests for our auto-encoding rewrite, but we are removing this now.
- We were doing this so that the runtime qubit allocator within QCorrect had all physical qubits reserved in advance (not in a logical state) and QAlloc caused it to prepare them in a logical state (see current main branch).
- Instead, in this PR, Henry changed it so that the runtime allocator has
Option[Iceberg[k]].
I prefer the latter approach (same as what Alec did), so that a user of this extension (including our auto-encoding pass) can only think of allocation and lifetime of logical blocks. This is for safety reasons: I don't want users to be able to pretend that a bunch of physical qubits is a logical block before it has gone through AllocZero or another preparation circuit (i.e. before it has been put into a state in the codespace).
I could be convinced to have something like Doug proposes if we had a different type DirtyBlock that AllocZero consumes to produce an IcebergBlock. That'd be similar to the notion of Money from BRAT, where you reserve the resources for a Qubit, but it's not a valid type to give to quantum operations. I can see the potential of something like this in the long run, but until we see the need for Money in Guppy, I think it's premature to add it here.
@hsemenenko keen to hear your thoughts on this. You mighty have a more pragmatic perspective on why what Doug proposes could be useful in the near future.
| /// Ry gate on all but one qubit. | ||
| all_but_one_ry, | ||
| /// Ry gate on all but one qubit (with dynamic index). | ||
| all_but_one_ry_d, |
There was a problem hiding this comment.
When going through my cleanup of Iceberg, I realised that all_but_one_ry is not implemented correctly. I do not know of a "native" implementation of this logical op at the physical level. Would you mind removing it? 😅
Closes #1400 .
I was in two minds about putting this in the tket2 repo or making a new repo for logical code extensions; still open to persuasion either way. Of course, the follow-up to this is compiling from computational to logical and from logical to physical; and that probably does belong in tket...