Sequence unrolling with QM #738
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## octaves #738 +/- ##
==========================================
Coverage ? 65.02%
==========================================
Files ? 50
Lines ? 6025
Branches ? 0
==========================================
Hits ? 3918
Misses ? 2107
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| self.operation: str = ( | ||
| f"{pulse_type}({self.pulse.duration}, {amplitude}, {self.pulse.shape})" | ||
| ) |
There was a problem hiding this comment.
Just to understand: here you're not using the ID because you explicitly want to deduplicate? I.e. treat different pulses with similar features (essentially the same waveform, on possibly different channels and timing) to be treated as a single one.
Or is there any other reason?
There was a problem hiding this comment.
Or is there any other reason?
No other reason, only deduplication. In principle this is not strictly required at the pulse level, because the waveforms are registered separately in the QUA config and you can have multiple pulses using the same waveform, but I think even better if we deduplicate even the pulse section whenever possible.
Also readout pulses are a bit more delicate because they are associated with variables (where we save acquisition) and these need to be deduplicated as well (so that we don't have multiple variables for the "same" readout when unrolling).
Of course the exact string format I used is not relevant. Any kind of id (readable or no) that does not duplicate the start/channel and is of string type works.
There was a problem hiding this comment.
No other reason, only deduplication. In principle this is not strictly required at the pulse level, because the waveforms are registered separately in the QUA config and you can have multiple pulses using the same waveform, but I think even better if we deduplicate even the pulse section whenever possible.
I wanted to understand because I still believe that deduplication should happen in the common layer, so it should not be a QM specific feature.
The goal would be to assign an ID per waveform at a higher level.
However, this is something for after 0.2, for the time being it is perfectly to have this feature here.
(I also wanted to understand which pulse attributes you were using to dedup)
There was a problem hiding this comment.
Of course the exact string format I used is not relevant. Any kind of id (readable or no) that does not duplicate the
start/channeland is of string type works.
I also don't care about the specific format.
For the sake of compression, you could hash() the attributes, but I'm pretty sure it's worth nothing.
| self.manager = QuantumMachinesManager(host=host, port=int(port), octave=octave) | ||
| credentials = None | ||
| if self.cloud: | ||
| credentials = create_credentials() |
There was a problem hiding this comment.
Interesting, I guess it's asking the device for the credentials, isn't it?
In any case, it's an interesting reading:
In [28]: s = create_credentials()._credentials._channel_credentials._pem_root_certificates.decode()
In [29]: s.count("END CERT")
Out[29]: 141If you filter the issuers with print("\n".join([line for line in s.splitlines() if "Issuer:" in line])) you could find any sort of company/institution in there...
# Issuer: CN=GlobalSign Root CA O=GlobalSign nv-sa OU=Root CA
[...]
# Issuer: CN=Hellenic Academic and Research Institutions RootCA 2015 O=Hellenic Academic and Research Institutions Cert. Authority
[...]
I guess is quite the standard stack of CAs... (I'm not expert enough on this...)
There was a problem hiding this comment.
Thanks, that's interesting, I don't know much about it either. Here it was just moved here from another existing file that I dropped (simulator.py). Initially it was also just copied from a tutorial. It is only needed when using a cloud simulator provided by QM, not for the real instrument. Btw this simulator is not working for a while now, so we could even drop, I just left it in case it comes back in the future (because it is useful for some quick schedule tests).
There was a problem hiding this comment.
I don't mind too much about keeping it, for the time being. It's already there and quite a small overhead.
It was more of a curiosity.
(I was also interested in whether we could hijack some data and use the simulator in the CI, without having access to the instrument. But if the simulator is not working anyhow is useless)
|
Thanks @alecandido. I addressed the review comments. |
Just ask again for a review whenever you need to merge. |
Please have another look, when you have time. I would like to merge this and #738 particularly because QM is currently connected to a single-qubit chip (not sure for how long) and people who are not core devs are using it, so it is better to not have them jump around branches. It also works as expected with the QPU, modulo some performance issues with unrolling. I was already aware of these and I am thinking of potential solutions but that's for another PR. |
| acquisition.assign_element(qmpulse.element) | ||
| acquisitions[name] = acquisition | ||
|
|
||
| acquisitions[name].keys.append(qmpulse.pulse.serial) |
There was a problem hiding this comment.
I'm not reviewing the whole module (maybe I should), but I wonder whether it is worth to have this nested structure (dictionary on names, and internally a list of keys), instead of unrolling to a single level.
There was a problem hiding this comment.
Of course, this depends on .fetch_all() requirements: if you have to call it on a given name, and it is much more efficient than individual .fetch() (assuming it exists), then there is no wiggle room for anything else.
There was a problem hiding this comment.
I think the structure is single level: each readout pulse is mapped to an Acquisition object. However multiple readout pulses may be mapped to the same Acquisition.
We could implement it with a single dictionary {ro_pulse: acquisition}, I just think the current implementation which is basically the inverse of this dictionary, is more useful for the manipulations we do later on these objects. I guess it appears as nested because it is the inverse of a map that is non-invertible (in the math sense).
| for serial, result in zip(acquisition.keys, data): | ||
| results[acquisition.qubit] = results[serial] = result |
There was a problem hiding this comment.
Aren't you overwriting results[acquisition.qubit]?
acquisition is always the same for all result (outer loop), and then acquisition.qubit as well. Isn't it?
There was a problem hiding this comment.
Aren't you overwriting
results[acquisition.qubit]?
Yes, but I think this is a more general problem with qibolab that extends beyond this driver.
Particularly, I am not sure how results[qubit] (here results is what is returned by platform.execute_pulse_sequence) is defined if the sequence contains more than one measurements on the same qubit. I had the impression that results[qubit] will only contain the last measurement and earlier measurements can only be accessed through their serial.
For now, I think I would leave it as it is here, and I will open an issue (#809) for a general solution in 0.2.
There was a problem hiding this comment.
That's perfectly fine. I was not aware of the problem. Thanks for the issue
|
@alecandido thanks for the second review. I will merge this to the |
Some updates in the QM driver for unrolling to work properly. In the existing implementation multiple acquisition variables are created for the same readout pulse (when unrolled, therefore copied multiple times) and this complicates the QUA program and increases execution time. This is fixed here.
Some benchmarks are still needed to confirm that the speed-up from unrolling is comparable to other instruments and to check what are the limitations in the sequence length or measurements per sequence.
Checklist: