-
Notifications
You must be signed in to change notification settings - Fork 2
feat!: add measure_leaked and bool/uint future results #25
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
Conversation
| struct Measurement { | ||
| measured: bool, | ||
| value: bool, | ||
| struct FutureResult { |
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.
Should we use the naming LazyMeasurement instead of future here and everywhere below, to stay consistent with tket2 & eldarion?
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 decided to move away from tying futures specifically to measurements in case we have other uses of futures, as the abstraction seems general enough for it.
In the Helios sense I think measurements are a good description, but Selene is aiming for a more general concept of devices/runtimes so I want to make it applicable for a wider range.
An example may be an extension to guppy/tket/etc that targets some other ecosystem and expects a runtime that can provide the current timestamp. This would be a u64 future (presumably one that flushes everything such that the timestamp corresponds to the same point in the guppy code), and could directly use the existing future type in a meaningful way.
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.
Ah yes, Selene is supposed to try to be "platform agnostic" to some extent that's right.
| }); | ||
| Ok(result_id) | ||
| } | ||
| fn measure_leaked(&mut self, qubit_id: u64) -> Result<u64> { |
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.
In guppy/tket2, measure_leaked actually returns a future value. Shouldn't we specify FutureResult here too?
(also apologies / thanks for your patience in advance -- much of my review is probably going to be me just asking clarifying questions)
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.
This is over the FFI boundary, so the underlying is a u64 value that provides a reference to an ID to poll. I should indeed add a u64Future type, typedef'd to a u64, to make this clearer.
|
This looks mostly fine to me. How tightly are you trying to tie the interface to the guppy semantics? I only ask because guppy Just left a few sporadic questions and small nits about |
| results.set_measurement_result(result_id, measurement); | ||
| results.set_bool_result(result_id, measurement); | ||
| } | ||
| Operation::MeasureLeaked { |
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.
If Selene is supposed to be hardware agnostic, is there a reason we are specifying MeasureLeaked as its own operation? especially if we are not modeling leakage? A more abstract alternative could be to have MeasureBool and MeasureUint if we care about return type.
I thought leakage heralding measurement was something specific to trapped ion computers that use a sympathetic coolant?
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 was apprehensive about adding this for the exact reason you have given. If it were kept internal I might have been able to justify adding measure_leaked through a plugin, or broken the measurement API to provide it with appropriate tweaks.
However, that gets very involved, and would require inelegant workarounds for the missing u64 future handling. It feels as this is now a standard feature of QIS that I've decided to live with it and maybe reconsider when Selene needs to make a break (e.g. expanding the gateset to beyond rxy, rzz, rz)
…gh the relevant interfaces
699ddb0 to
3e17675
Compare
This reflects a change to the helios QIS, as well as the interface to the runtime and error model interfaces.
Code changes
measure_leaked should produce a future value that can be fetched as a u64, rather than as a bool that we currently have in place.
I was torn between keeping read_result as deprecated, as now encoding the type in the name is more appropriate. I have decided against that, that we do not provide API stability guarantees yet, and keeping it around might be confusing, so I have discarded the old interface.
This does, however, include a bump to the error model and runtime API versions, so old versions should be detected and rejected rather than causing UB.
Implementation
selene_qubit_lazy_measure_leaked.measure_leaked_request_countmetric in the user_program categorymeasure_leaked_batch_countandmeasure_leaked_individual_countmetrics appropriately.> 0comparison, and from a boolean by aif bool { 1 } else { 0 }expression.Wait to merge
Depends on: