-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CalibrationSet Python bindings #156
CalibrationSet Python bindings #156
Conversation
implement rest of CalibrationSet bindings
…libration-set-api
quil-py/src/program/calibration.rs
Outdated
pub fn get_match_for_measurement( | ||
&self, | ||
py: Python<'_>, | ||
measurement: PyMeasurement, | ||
) -> PyResult<Option<PyMeasureCalibrationDefinition>> { | ||
Ok(self | ||
.as_inner() | ||
.get_match_for_measurement(&Measurement::py_try_from(py, &measurement)?) | ||
.map(PyMeasureCalibrationDefinition::from)) | ||
} | ||
|
||
pub fn get_match_for_gate( | ||
&self, | ||
py: Python<'_>, | ||
gate_modifiers: Vec<PyGateModifier>, | ||
gate_name: &str, | ||
gate_parameters: Vec<PyExpression>, | ||
gate_qubits: Vec<PyQubit>, | ||
) -> PyResult<Option<PyCalibration>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Corresponding to this comment)
Two things here:
get_match_for_measurement
takes a measurement as a single param, butget_match_for_gate
requires the caller to decompose the gate. Why's that?Could this just beI see the benefit of having a narrower type in the return signature.get_match_for_instruction
and do the match on this side? Keeps more of the logic together in this repo rather than farming some of it out to pyQuil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a question I was going to pose once this was out of draft, but worth considering now:
To explain why they are different, get_match_for_gate
was a pre-existing method, so I just mirrored it in the bindings. get_match_for_measurement
was tied up in expand
, so I had to lift it out and chose to just use a Measurement
directly, rather than decompose it.
My preference is to take the latter approach, so I would like to update get_match_for_gate
to take a Gate
instead of the individual components. You happened to write get_match_for_gate
, so I'll reverse the question back on to you: Is there a benefit to having the caller have to decompose the Gate
rather than just pass a Gate
object in? If not, then I'll make the change. If there is a benefit then I'll update get_match_for_measurement
so that it is consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored get_match_for_gate
to use a &Gate
and labeled the commit as a breaking change.
Ports all of the
CalibrationSet
methods to Python bindings.I uncovered #158 while working on this. The corresponding fix (#159) needs to be merged before this.