Repository navigation
feat!: add support for named RESETs - #532
mingyoungjeng wants to merge 16 commits into
Conversation
|
I'm a fair bit skeptical we should define a separate calibration type for reset calibrations. In contrast to measurements, where there is an (optional) "memory out" parameter, a reset calibration can be expressed in the same way as a gate calibration (granted, reset instructions currently don't accept read-only parameters, we shouldn't presume that will always be the case). More important than some of the implementation details I'll advocate below, the general perspective I'd pitch is:
Given this perspective, I would advocate an implementation along these lines:
|
Assuming (named) reset calibrations should operate like current (named) measurement calibrations, i.e., with the from a type-safety perspective, these should be distinct identifiers from gate calibrations. In more concrete terms, the differences between a (gate) struct CalibrationIdentifier {
name: String,
qubits: Vec<Qubit>,
parameters: Vec<Expression>,
modifiers: Vec<GateModifier>,
}
struct ResetCalibrationIdentifier {
name: Option<String>,
qubit: Option<Qubit>,
}
I think there is discussion to be had on generalizing calibration definitions (and Quil's other
I think I can attempt to implement calibrations with something like this: enum CalibrationIdentifier {
// Rename current `CalibrationIdentifier` => `GateCalibrationIdentifier`
Gate(GateCalibrationIdentifier),
Measure(MeasureCalibrationIdentifier),
Reset(ResetCalibrationIdentifier),
}
struct Calibrations {
calibrations: HashMap<CalibrationIdentifier, Vec<Instructions>>,
}This would put calibrations in a single map without compromising on the type guarantees in the existing implementation. |
|
edyounis
left a comment
There was a problem hiding this comment.
The separation of CalibrationIdenifiers seems like an unnecessary abstraction. There is a lot of repeated code and enum dispatch to include to the separation, when the real logic is that the different structures have different requirements on signatures. The simplest way to include that logic would just be a check during parsing, rather than internalizing it in the structure of the program. There is also the extension of the name in for quil writing purposes. A clue to this is that write_calibration_definition calls identifier.write and instruction.write
This is definitely me saying, "I would have designed it differently," and I want to avoid the usual second part of that statement: "So you should too." So, feel free to ignore this point, it won't stop me from approving this, but I wanted to bring it up. Everything you have done looks correct.
Also the renaming of CalibrationDefinition to GateCalibrationDefinition is a breaking API change. I agree with it, but it is unnecessary and worth calling out. I really do agree with it, and doubt it will effect anyone.
| let Self { | ||
| identifier, | ||
| instructions, | ||
| } = self; |
There was a problem hiding this comment.
NIT: feel free to ignore, but why destruct here? Why not just &self.identifier, &self.instructions?
There was a problem hiding this comment.
For me, it is mostly to illustrate which fields of the struct are being used and to automatically flag if one of those fields changes in the future.
| qubit.write(writer, fall_back_to_debug) | ||
| } | ||
| None => write!(writer, "RESET").map_err(Into::into), | ||
| let Self { name, qubit } = self; |
There was a problem hiding this comment.
Same comment as above, why destruct? (Feel free to ignore)
| input: ParserInput<'a>, | ||
| ) -> InternalParserResult<'a, Instruction> { | ||
| let (input, modifiers) = many0(parse_gate_modifier)(input)?; | ||
| let (input, name) = token!(Identifier(v))(input)?; |
There was a problem hiding this comment.
Is the ! a separate token that would fail this? If not and it can find its way into an Identifier, do we want to include some logic to catch and nudge the user back to the correct way, for example in a miscapitialization case like reset!blah
There was a problem hiding this comment.
That is probably worth doing at some point, and I think there are a few areas in the parser where messages like that could be provided.
| } | ||
| Instruction::RawCapture(raw_capture) => { | ||
| self.instructions.push(Instruction::RawCapture(raw_capture)); | ||
| body_instruction => { |
There was a problem hiding this comment.
I am missing something, where does this match case come from? I can find the body_instruction symbol
There was a problem hiding this comment.
This is just a wildcard catch
Adds support for named reset calibrations, akin to named measurements (quil-rs!479)
ResetCalibrationDefinitionandInstruction::ResetCalibrationDefinitionRESETto a gate-level instruction, which can be expanded with calibrationsCalibrationDefinitionandCalibrationIdentifiertoGateCalibrationDefinitionandGateCalibrationIdentifier, respectivelyCalibrationSourcetoCalibrationIdentifierInstruction::get_qubits,Instruction::get_qubits_mut, andProgram::used_qubitsTODO: