Instruction API: Calibrations #155
Conversation
quil-py/quil/instructions/_qubit.pyi
Outdated
| - is_*: Returns ``True`` if the inner type is of that variant. | ||
| - as_*: Returns the inner data if it is the given variant, ``None`` otherwise. | ||
| - to_*: Returns the inner data if it is the given variant, raises ``ValueError`` otherwise. | ||
| - from_*: Creates a new ``GateSpecification`` using an instance of the inner type for the variant. |
There was a problem hiding this comment.
| - from_*: Creates a new ``GateSpecification`` using an instance of the inner type for the variant. | |
| - from_*: Creates a new ``Qubit`` using an instance of the inner type for the variant. |
Co-authored-by: jselig-rigetti <97701976+jselig-rigetti@users.noreply.github.com>
Co-authored-by: jselig-rigetti <97701976+jselig-rigetti@users.noreply.github.com>
Co-authored-by: jselig-rigetti <97701976+jselig-rigetti@users.noreply.github.com>
Co-authored-by: jselig-rigetti <97701976+jselig-rigetti@users.noreply.github.com>
…/rigetti/quil-rs into 154-instruction-api-calibrations
|
@jselig-rigetti Nice catches! Fixed. |
| impl MeasureCalibrationDefinition { | ||
| pub fn new(qubit: Option<Qubit>, parameter: String, instructions: Vec<Instruction>) -> Self { | ||
| Self { | ||
| qubit, | ||
| parameter, | ||
| instructions, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This doesn't seem necessary if all of the fields are public.
There was a problem hiding this comment.
Maybe not necessary, but it makes the API more consistent if all the instruction types have new methods. Making the members of the instructions private is up for consideration as well since manual manipulation of the fields could easily malform an instruction. That would be a big change, but having and using constructors allows us to make it much easier in the future.
There was a problem hiding this comment.
Yeah, this approach is definitely a change in philosophy here. Previously/until now, these were really just data objects, which assert the type of the data held within but otherwise not its validity in any scope. So a parameter could be an invalid identifier and that was just fine, until the program was written to string and then read again. Both ways are valid, but this newer approach does make sense as the library matures away from being parser-focused.
This approach, with new, does add some boilerplate to the library, and I wonder if derive_builder would be more flexible and future proof without significantly more work. In fact, it might be less work since it's a derive rather than a hand-written new method for each instruction. If something internal to the struct changes, a builder is less likely to break than new. It also neatly supports validation.
More importantly, users do need to be able to mutate these instructions; that's a common pattern for users of pyQuil. So no matter what, we need to support the ability to do that from pyQuil and likely for Rust users as well.
- Today, that could be operating directly on the public struct fields via
pyO3; - if we added
newhere and made the fields private, we'd need at a minimum some way to read the fields and then send back tonewfor an updated copy; - if we use the the builder pattern, the user would turn a
Calibrationback into aCalibrationBuilder, update the field, and re-build()
Overall though I don't want to bike shed over it - I'll defer to @MarquessV 's judgment here.
There was a problem hiding this comment.
To follow up here after further discussion with @kalzoo.
We are going to continue with the new pattern used here. It is fast and allows us to do everything we need to do to support the next pyQuil RC. Long term we'll look at something like derive_builder as we agree that is closer to an ideal solution, but would take much longer to port the builders and associated methods to Python.
| impl MeasureCalibrationDefinition { | ||
| pub fn new(qubit: Option<Qubit>, parameter: String, instructions: Vec<Instruction>) -> Self { | ||
| Self { | ||
| qubit, | ||
| parameter, | ||
| instructions, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Yeah, this approach is definitely a change in philosophy here. Previously/until now, these were really just data objects, which assert the type of the data held within but otherwise not its validity in any scope. So a parameter could be an invalid identifier and that was just fine, until the program was written to string and then read again. Both ways are valid, but this newer approach does make sense as the library matures away from being parser-focused.
This approach, with new, does add some boilerplate to the library, and I wonder if derive_builder would be more flexible and future proof without significantly more work. In fact, it might be less work since it's a derive rather than a hand-written new method for each instruction. If something internal to the struct changes, a builder is less likely to break than new. It also neatly supports validation.
More importantly, users do need to be able to mutate these instructions; that's a common pattern for users of pyQuil. So no matter what, we need to support the ability to do that from pyQuil and likely for Rust users as well.
- Today, that could be operating directly on the public struct fields via
pyO3; - if we added
newhere and made the fields private, we'd need at a minimum some way to read the fields and then send back tonewfor an updated copy; - if we use the the builder pattern, the user would turn a
Calibrationback into aCalibrationBuilder, update the field, and re-build()
Overall though I don't want to bike shed over it - I'll defer to @MarquessV 's judgment here.
Closes #154
This PR includes type hints for
CalibrationandMeasureCalibrationDefinition, I also added type hints for a lot of what was already existing, so that future MRs only contain type hints that are relevant to whatever that MR is touching.