Qubits, channels, and acquisition #1001
Conversation
|
@stavros11 I still need to:
Other than these points, this PR is almost ready, but there will be two points left. The trivial one is the one that makes the C API failing (and potentially any similar situation): I'm now type hinting the base LO class with Then the non-trivial point, that I still want to address in this same PR is QM: I'm making some arbitrary choices about how to handle channels, and the only two places that are giving me some feedback are the dummy platform and the compiler. Which are both pretty limited. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 0.2 #1001 +/- ##
==========================================
- Coverage 51.86% 51.45% -0.41%
==========================================
Files 58 57 -1
Lines 2765 2740 -25
==========================================
- Hits 1434 1410 -24
+ Misses 1331 1330 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
The usual upgrading script, to convert import sys
import json
from pathlib import Path
from typing import Optional
path = Path(sys.argv[1])
run = json.loads(path.read_text())
def upgrade(
gate: str, seq: Optional[list[tuple[str, dict]]]
) -> Optional[list[tuple[str, dict]]]:
"""Upgrade sequence layout.
From https://github.com/qiboteam/qibolab/pull/971 to
https://github.com/qiboteam/qibolab/pull/970
"""
if seq is None or gate != "MZ":
return seq
return [
(seq[1][0], {"kind": "readout", "acquisition": seq[1][1], "probe": seq[0][1]})
]
for section in run["native_gates"].values():
for qubit, gates in section.items():
for gate, sequence in gates.items():
gates[gate] = upgrade(gate, sequence)
path.write_text(json.dumps(run, indent=2)) |
|
I'm missing the QM update, which will be a review on its own. But other than that the features should be all there, so a first look and early opinion could be already useful :) (both of the points here, #1001 (comment), are still relevant - that's why the C API is still failing) |
There was a problem hiding this comment.
Thanks @alecandido. I had a quick look and it generally looks better than the previous acquisition PR. ChannelId should be simpler to handle as a string (although for this I may be a bit biased from QM) and dropping the names from channels should also be a simplification. My only concern so far is about Readout and generally the measurement interface (last comment below).
The trivial one is the one that makes the C API failing (and potentially any similar situation): I'm now type hinting the base LO class with
qcodes. This implies that, if you import the LO module,qcodeshas to be available. But that is also used byDummyLocalOscillator, which in turn is used by the dummy platform. Should we make aDummyLocalOscillatornot inheriting from the base class? Or make an empty (and useless) base class independent ofqcodes? Or just give up on theDummyLocalOscillator? Or keep it, but split from the dummy controller in a separate module?
The DummyLocalOscillator should now be used only by the dummy platform. I think before the ZI platform was also using it to set internal LOs, but since we now have the channel configs, it shouldn't be needed there anymore. So dropping could be possible, but we may still want to keep for the dummy to have an LO so that it is closer to real platforms.
If we keep it, it should certainly be free of optional dependencies such as qcodes. I don't think moving the dummy controller to a separate module would help, as if the dummy is still using DummyLocalOscillator directly, we don't want it to depend on qcodes. Rearranging the inheritance should work, for example
graph TD;
Base-->Dummy;
Base-->Real;
Real-->R&S;
Real-->EraSynth;
where Base and Dummy are qcodes-free. In that scheme I would try to move shared methods as upwards as possible (to avoid repetition), but I am not sure to what extent this is possible.
Then the non-trivial point, that I still want to address in this same PR is QM: I'm making some arbitrary choices about how to handle channels, and the only two places that are giving me some feedback are the dummy platform and the compiler. Which are both pretty limited. Thus, I want to update QM in this same PR, to possibly iterate. If you want to do something on your own, feel free to push here. But I can also do it myself.
Usually, I also get a better feeling of how everything works when I try to implement it for QM and update the real platform and some qibocal routines. I think we should do that before merging. Let me know if you are planning to do it yourself in case you also want to have a look, otherwise I can also do it (just let me know to avoid doing in parallel). From my side, I was planning to add the missing QM features starting from the working branch (#979), which is pretty much indepedent and I can rebase to this later. Most likely after tomorrow, though.
| drive_qudits: dict[TransitionId, ChannelId] = Field(default_factory=dict) | ||
| """Output channels collection, to drive non-qubit transitions.""" |
There was a problem hiding this comment.
I understand that this is a generalization of the previous drive12. I am not sure how useful are higher order transitions in practice, but should be fine, why not provide support anyway.
There was a problem hiding this comment.
It looked to me more polished to have the general object.
Moreover, the other more subtle reason for doing this is to keep some "flexible space" for channels, to be abused.
It doesn't sound great, and if not needed it should not be used, but there might be occasions where it could be handy to associate extra channels to qubits in Qibocal. To avoid any possible breaking change in the short term, I left some room here, such that it could be used as a workaround (e.g., you can use the transition 0-1000, 0-1001, ... with a custom Qibocal encoding).
If any of these patterns will arise, we should then stabilize it with proper support. But possibly in 0.3. So, some extra wiggle room for such a huge change as 0.2 doesn't hurt.
There was a problem hiding this comment.
I assume there were no actual requests, just a comment. In case I misinterpreted, please reopen the conversation.
There was a problem hiding this comment.
Providing some flexible space sounds like a good idea. If that is the case, we could even make it more apparent that this is such a thing. For example, instead of the tuple (TransitionId) type, we could use just str for the keys and even have a less descriptive name, such as additional, instead of drive_qudits.
That being said, I am not sure if there are any "additional" channels that are not associated to some transition that could be relevant for some actual application, to motivate relaxing the TransitionId structure. One example could be that we can do the 0->2 using one or two photons (with half the frequency) but I think that single photon 0->2 frequency cannot be reached by our electronics so it's probably not useful anyway.
There was a problem hiding this comment.
Possibly related to this discussion: what was implemented in #907 could be an example of an "additional" pulse that is not strictly associated to a drive to an excited state.
There was a problem hiding this comment.
Ah, yes, for that we should augment even the probe channels.
But I'd still keep as it is, for the time being. We're lacking person power for the full qudits project, and scheduling for 0.3 should not be that limiting.
The extra wiggle room here would still allow doing something like that, e.g. using the 0-1001 encoding for that is an option for the probe, since it's still an IqChannel (though you will lack a channel for the acquisition...).
@stavros11 please, keep going with the other features, as I won't touch those. I will now try to update QM here by myself (just because I'd like to have a more direct feedback). In case of troubles, or lack of time, I'll get back to ask your help. |
|
@stavros11 I'm still waiting qiboteam/qibolab_platforms_qrc#170 for testing, but most of the changes I made to QM should be a trivial consequence of the main one: I discarded The reason I made this is that, in the previous commits, I started requiring the general channels to be stored in the instruments themselves (specifically, in the Another bunch of changes are triggered by the removal of the name within the channels. If needed, now it has to be passed separately. The only bit about which I'm unsure is within |
alecandido
force-pushed
the
qubits
branch
2 times, most recently
from
d0f6354 to
fee41c7
Compare
That's great. I have not checked the details of the implementation, but I guess that at some point you'll have to cast the
If we are using "D2/probe": {
"digitalInputs": {
"output_switch": {
"delay": 57,
"buffer": 18,
"port": ('con6', 1),
},
},
"digitalOutputs": {},
"outputs": {
"out1": ('con6', 1),
"out2": ('con6', 2),
},
"time_of_flight": 224,
"smearing": 0,
"intermediate_frequency": -70432000.0,
"operations": {
"-6192737274986805398": "-6192737274986805398_D2/probe",
},
"mixInputs": {
"I": ('con6', 1),
"Q": ('con6', 2),
"mixer": "D2/probe_mixer_18b",
"lo_frequency": 7450000000.0,
},
},Its name doesn't really matter, as long as we are consistent between the config and the program. However it should contain all this information, which in qibolab is spread in two different channels: for example frequency is on the probe channel, while time of flight and smearing are on the acquisition channel. Therefore, I would guess that you need to process both channels at the same time to properly generate this. Registering additional elements in the config that are not used in the program is fine, as in there will be no errors and everything will run as expected. You may just end up uploading more stuff than needed in the device, which may cause some overhead, but most likely negligible. I am not sure why you need to go through this process of registering and unregistering though, instead of just registering only the elements we really need in the first place, but I have not looked at the code in detail yet. |
There is now a |
This is feasible, as
Here the problem is that I can not distinguish a probe channel from a drive one any longer. They are both https://github.com/qiboteam/qibolab_platforms_qrc/blob/aae486cf3cc73c5379561191e96ef69bfd3c955c/qw11qD/platform.py#L40-L44 |
Thanks for the explanation. I guess the only way to distinguish them now is that probes will be referenced by some acquisition channel. We could probably use that to avoid registering them as drives. Unrelated, but looking at the platforms, |
Yes, but if you follow the current strategy, you should sort all the
I was missing a better name, so I kept the Zurich one. I'm fully open to proposals. |
Make type check useful, to catch this error later on
|
Ok, now I have a report for single shot, using this PR and Here the report: Now, we need to test routines including sweepers. |
|
Thanks @alecandido. With d5b7c6f and the latest push in qiboteam/qibocal#973 I managed to run the Rabi routines, which use amplitude and duration sweepers.
I will try to do a more detailed review tomorrow or Monday. |
|
One issue I see, is that when I add the measurement delay in the probe channel, I get the usual QM error, while if I use acquisition everything works. From the user (who has no idea how QM works) point of view, this may be a bit confusing, as they would probably expect that a delay in the probe only delays the probe etc.. I get that in this case the delay should be added in the acquisition, because the Regarding the QM error in this particular case, I did not explore it in detail, but could it be because you are defining two elements ( |
Another point about this, not directly related to this PR, is that we should be a bit careful with what we advertise as the measurement API, particularly for qibocal purposes. If qibocal is using pulses/delays on probe, independently of acquisition, hoping that some instrument (not the current QM) will support them, then all these routines won't be supported by QM. On the other hand, if we stick to using I remember there were discussions about how to advertise what each instrument supports, but eventually the goal of qibolab-qibocal is to run the same routines on as many different platforms as possible, and to achieve this we probably have to limit ourselves to the subset of features supported by all instruments. |
At this point, my advice is to keep
Yes, but here is the point where I want to make a step forward wrt the previous aim: before we were trying to support all routines everywhere. And thus, we were exposing a minimal Qibolab API, that should have worked on all the instruments (or almost all). Instead, I'd like to move Qibolab towards the union of features, but somehow declaring which is the intersection (e.g. you can compare to the compatibility tables on MDN). |
There was a problem hiding this comment.
Thanks @alecandido. The only significant issue I found is one about the channel sweepers (offset and frequency) in QM which should be broken (comments below). However, I can fix it myself if you agree, I will push here. The rest are minor comments and some suggestions.
One other point, mentioned somewhere else before, is about drive_qudits and whether we would like to convert it to something more flexible, such as a dict with str keys instead of TransitionId. Personally, I don't have a strong opinion, I suggested the str because it is more open to other uses that we may not be aware right now, however I see the advantages of having something more structured for transitions. Whatever we decide will be fixed for all 0.2.* releases as it is certainly in the public interface.
Yes, for this I don't have any definitive answer. The usage of |
Co-authored-by: Stavros Efthymiou <[email protected]>
As they are now embedded in the parameters.json
As they are already in the sweepers
|
@stavros11 I fixed all the simple issues, and left you with the channels in the sweepers. |
There was a problem hiding this comment.
QM sweeper issues should be fixed by 585eb0b. Frequency sweeper works locally, I have not tested on hardware. Maybe it is worth updating the resonator spectroscopy and flux routines to test on hardware, but I need to do this in qibocal.
Other than that, this is fine to merge from my side. @alecandido maybe you want to review the last commit because I tried to also simplify a bit (instead of just converting to tuple as I would need to propagate this to several places. If you agree, feel free to merge.
The usage of
TransitionIdwas motivated by the fact that it should be used mainly for that purpose, and the flexibility is just a safety option. On a practical level,int(andtuple[int, int]) is as much unlimited asstr, so it could fit whatever you wish. But one or the other could be more convenient according to the main use, which I'm unsure which it will be...
I am not sure either. We could ask others if they have any idea what additional channels could be useful and if they all fall under the transitions regime. But I would say the best option for now is to leave as it is and move to an issue/question to potentially address in later releases. It is not really interface breaking. If there is really need for additional channels and the tuple is not convenient to define them, we could just add an additional attribute in some 0.2.* and leave its better refactoring for 0.3.
| @property | ||
| def port(self) -> int: | ||
| return int(self.path) |
There was a problem hiding this comment.
I don't think this would work for all instruments, at least probably not for ZI, so we could even "downgrade" internally to QM. It could be useful for other instruments though, which could justify keeping it here.
There was a problem hiding this comment.
ZI will use the .path, and most of the others will also use .path directly.
I would say that it makes a bit of sense, since int and str are the only general types we can accommodate for, using str to represent both. If any driver needs anything more structured, it will make more sense to write the custom (de)serialization (from)to str in the driver itself, and treat it as str in the channel.
We could follow this logic for int as well, but that's the minimally structured one, so we can even afford (it's just a compromise).
Yes, that's also another good reason: the structured
I'm checking and merging right after. It is worth to test more with Qibocal, but it's also quite time-consuming. If some routine breaks, the fix will be most likely internal to the driver, so it's worth to keep going. |
It seems simpler to me as well, I'm merging. |
Pulsefrom another one (not even a probe one, we look forAcquisitionorReadoutfor their special needs)Readoutand simplify its creationChannelandQubit)AcquisitionChannel.probeandIqChannel.acquisitionChannelId, it could make sense to keep them, or not evenReadoutinstructions (or probePulseseparate fromAcquisitionin general)