[STIM] Finalize non-Clifford support - #3647
Conversation
| } | ||
|
|
||
| #[test] | ||
| fn pauli_channel_1_with_probability_in_radians_yields_error() { |
There was a problem hiding this comment.
Could you add a test where more than one argument is expressed in radians? Should multiple errors surface, or will it just be one error for the whole call?
| } | ||
|
|
||
| #[test] | ||
| fn peek_loss_with_readout_noise_in_radians_yields_error() { |
There was a problem hiding this comment.
What about a test for an argument -0.1rad? That way we can test for both errors surfacing instead of one "beating" the other.
| args: | ||
| Arg [2-6]: | ||
| value: 0.01 | ||
|
|
There was a problem hiding this comment.
Can we avoid having this extra line space here? It breaks off the "targets" field and makes it look like it is unrelated to the rest of the fields of Instruction.
|
|
||
| impl Display for Arg { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| writeln_header_with_span(f, "Arg", self.span)?; |
There was a problem hiding this comment.
The value field is pretty compact in its display. Consider making the Arg display a single line:
Arg [x-y]: 1.2 rad
| impl Display for Arg { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| writeln_header_with_span(f, "Arg", self.span)?; | ||
| writeln_field(f, "value", &self.value) |
There was a problem hiding this comment.
| writeln_field(f, "value", &self.value) | |
| write_field(f, "value", &self.value) |
I think using write_field like Target does will avoid that extra line space in the output.
| "source": [ | ||
| "## Controlled gates\n", | ||
| "\n", | ||
| "`CH` applies a Hadamard when its control is `1`. `CCX` flips its target, and `CCZ` applies a phase flip, when both controls are `1`." |
There was a problem hiding this comment.
It is a bit more nuanced than this makes it sound. How deep you want to describe this depends on the audience, but doing a quantum operation with a control means that the execution of the operation is entangled to the part of the quantum state of the control that is 1. There's probably a better way to word that then what I gave, but I assume there are nice descriptions of what quantum controls do out there.
There was a problem hiding this comment.
I have merged all of the stim notebooks into a single, more complete one. The idea isn't for it to be a full documentation of how all of the gates are supposed to work. For that, one can simply go to stim's docs. For now, this is supposed to be used by people who already use stim / clifft but aren't familiar with qdk.stim.
In this particular case, the description is explaining what the example is supposed to do, rather than making a general statement about those gates.
|
The non-Clifford gate support and lowerings look good. I have one blocking design concern: could we use half turns exclusively and remove the This matches Clifft’s model, including Concretely:
This also removes duplicated unit semantics across the lexer, AST, and validators, leaving one instruction-owned interpretation point. |
Domingo Morales Lizama (domorale)
left a comment
There was a problem hiding this comment.
Requesting changes based on the angle-unit design concern in my review comment. I consider this a user-facing compatibility and predictability issue: matching Clifft’s half-turn model avoids a QDK-specific Stim dialect.
|
Domingo Morales Lizama (@domorale), answering your comment: we decided to add the rad suffix after discussing it within the team and with the QEC researchers, mainly for two reasons:
|
Domingo Morales Lizama (domorale)
left a comment
There was a problem hiding this comment.
I requested changes earlier, but the discussion about opt-in rad support and compatibility clarified the design intent. I approve the approach.
This PR finalizes non-Clifford support in the STIM-to-QIR pipeline by adding the remaining gate lowerings, explicit radian angles, and CCX execution in the Clifford simulator.
STIM compiler
TPP/TPP_DAG,CH,CCX,CCZ,U/U3,R_XX/R_YY/R_ZZ, andR_PAULI.Doubletokens unit-aware throughDoubleUnit, distinguishing default values from radians before parsing.Arg { value, span }, preserving their units and source locations for compiler validation.radsuffix before conversion, and reports floating-point overflow at the argument span.Sample notebooks
qdk_stim.ipynb.Tests
Simulation
Editor support
rad-suffixed numbers in STIM files.