-
Notifications
You must be signed in to change notification settings - Fork 17
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
Qblox #1088
base: parse-q1asm
Are you sure you want to change the base?
Qblox #1088
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## parse-q1asm #1088 +/- ##
===============================================
- Coverage 54.36% 48.52% -5.84%
===============================================
Files 65 81 +16
Lines 3302 3872 +570
===============================================
+ Hits 1795 1879 +84
- Misses 1507 1993 +486
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
8ac151f
to
f7a8883
Compare
@stavros11 I start wondering why do we have this distinction at all: qibolab/src/qibolab/_core/components/channels.py Lines 32 to 35 in f74c334
Couldn't we have just a It would have made sense if Moreover, we internally distinguish channels only by their logical name (stemming from If we agree on this, what we could do is to deprecate The other option is to just deprecate it "among us", and open an issue for removal in 0.3. Since it already has a default value (empty string), we just ignore it in the drivers and that's it (which is what I'm planning to do right now). |
Yes, this should be possible. I guess the distinction is leftover from 0.1 and maybe was relevant up to some point in 0.2, but it is not anymore.
I think it will break the QM driver, as if channel.device is None:
device = # parse from `channel.path`
else:
device = channel.device
... It is not optimal, as I generally don't like providing two (equally trivial) interfaces to do the same thing, but I would be fine doing it only for QM, if it will simplify all other drivers. For other drivers, we can completly ignore |
Ah, yes, the idea was to absolutely do something like E.g. if you have a function that takes a In practice, I don't expect that there is any function like that (nor anyone using |
def _eltype(el: str): | ||
return "qubits" if el[0] == "q" else "couplers" |
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 know this is WIP and I am not sure exactly how it is used, but I would expect drivers to be qubit/coupler agnostic and only play with channels and configs, without caring exactly which qubit each channel controls, but maybe only the channel type.
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 just a support function for writing "simpler" platforms, which I moved from the current sketch of the platform.
The idea is that using this function is fully optional. But the function itself is opinionated, favoring certain layouts (hopefully, the most frequent ones), making it simpler to describe the connections.
An example of its usage is here:
https://github.com/qiboteam/qibolab_platforms_qrc/blob/e4011ded804135d758310082616860bb959d94fd/iqm5q/platform.py#L16-L27
(you can see that this is attempting to mirror the connections that you physically have in the lab, to make the definition simpler when you have that picture - the function is supposed to do the rest - see conventions and caveats in its docstring)
To be fair, the reason why I introduced this is that we're passing qubits
and couplers
separately in the platform, and consequently defining them in two separate sequences.
However, I refactored many times to improve the representation and implementation, and by now there is a single relic for that separation, i.e.:
qibolab/src/qibolab/_core/instruments/qblox/platform.py
Lines 93 to 96 in 1514f00
if kind == "qubits": | |
channels[el.acquisition] = channels[el.acquisition].model_copy( | |
update={"probe": el.probe} | |
) |
All the rest is already performed in the very same way.
So, since it is just post-processing, I should be able to concatenate qubits
and couplers
, and just post-process qubits
for the acquisition.
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 have not looked at the full content of the file in detail so maybe it is indeed useful, however my main concern (and what I don't understand) is why this is under qblox. I would expect the instrument to have no knowledge about qubits
and couplers
at all, as the Platform
never communicates these objects to the instrument.
If it is actually useful, it would make more sense to lift this outside the drivers and make it usable by any platform.
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.
As said above, these qubits
& couplers
issue I should be able to solve, and I will (try to) do it soon.
If it is actually useful, it would make more sense to lift this outside the drivers and make it usable by any platform.
We can consider generalizing it, with some pluggable choices. But for the time being it is tailored to Qblox in a few ways (not infinitely many).
The most outstanding example:
qibolab/src/qibolab/_core/instruments/qblox/platform.py
Lines 19 to 23 in 1514f00
if mod.startswith("qcm_rf"): | |
return "drive", IqChannel | |
if mod.startswith("qcm"): | |
return "flux", DcChannel | |
if mod.startswith("qrm_rf"): |
true that we could provide some mappings from module names. But not even all instruments have modules... (e.g. not QICK)
Let's say that I'm using Qblox to experiment the approach. But I'm not aiming to make something more general than that, at first shot.
And keeping it as a fully optional part, even for Qblox, we should be free to just leave it there at some point, and replace with some more general implementation (if general-enough, by reimplementing the current interface making use of the new one, otherwise by keeping both, and deprecating the Qblox-specific one).
@aorgazf @DavidSarlle apparently Qblox sequencers can be connected to be used for input, output, or both https://docs.qblox.com/en/main/api_reference/cluster.html#qblox_instruments.Cluster.connect_sequencer I'm taking into account both the input and output case (of course), but I don't see how the Do you have any use case for this |
Sorry for the late rely @alecandido. I have not seen before thi possibility before. And as you said I can not see when we can use the channel in both ways. But @aorgazf knows better than me the driver and he has some explanation or example where ports it can be used in that mode. |
f7a8883
to
442df19
Compare
6faee02
to
edf1dca
Compare
Update params during sweeps
|
Check ports assignment for microwave channels
(Pdb++) print({k: [(c, p.local_address) for c, p in v] for k, v in self._channels_by_module.items()})
{
2: [('0/flux', 'out1'), ('1/flux', 'out2'), ('2/flux', 'out3'), ('3/flux', 'out4')],
4: [('4/flux', 'out1'), ('c1/flux', 'out2'), ('c3/flux', 'out4')],
6: [('c4/flux', 'out2')],
8: [('1/drive', 'out1'), ('2/drive', 'out2')],
10: [('3/drive', 'out1'), ('4/drive', 'out2')],
12: [('0/drive', 'out1')],
17: [('c0/flux', 'out1')],
19: [('0/acquisition', 'in1'), ('0/probe', 'out1'), ('1/acquisition', 'in1'), ('1/probe', 'out1')],
20: [('2/acquisition', 'in1'), ('2/probe', 'out1'), ('3/acquisition', 'in1'), ('3/probe', 'out1'), ('4/acquisition', 'in1'), ('4/probe', 'out1')]
}
We do not use complex IO, since the mixing is happening internally. |
But incorrect mw ports assignment qiboteam/qibolab#1088 (comment)
@DavidSarlle (cc @aorgazf), concerning our previous discussion about the This would optimize the number of sequencers used, and it would practically always fit our use case, because the readout process (to which any measurement is translated) involves sending a pulse on the probe line (former readout line) and listen right after on the acquisition line (former feedback). While this may be interesting, it is certainly an optimization, and (if I didn't overlook it) it is not even supported by the 0.1 driver. Moreover, this should be declared in the platform (unless making further inference and optimization automatically - which is only another layer of complexity). [*]: the upstream Q1 processor may take some time to even process the instruction, so I may have to introduce some initial delay even before the beginning of the experiment, to make sure both are being processed before starting any - and I'm also not sure how they would be processed once queued in the real-time pipeline (I'm about asking this) |
442df19
to
d108ad5
Compare
8e1ee1c
to
e4ec32f
Compare
@alecandido thanks for the information and keeping us informed. All your comments make sense to me. As you said it is an optimization that we can try to implement when the driver is fully operational and when the rest of the priorities have been implemented. |
d108ad5
to
1b0d8d2
Compare
This is currrently conflicting with the given guidelines in #1074
qibolab/src/qibolab/_core/instruments/qblox/results.py Lines 91 to 99 in 15cafdc
requires a rather cumbersome computation. This has been introduced following the tutorials provided by the vendor, in which this information is retrieved from a different channel than the rest of the acquisition However, the collected mapping which is retrieved by |
This will be the prototype for the 0.2 Qblox driver, based on the Q1ASM handling layed out in #868
Progress
Program
serialization and deserialization (from JSON, not from Q1ASM)Sequence
Sequence
(which I'm also using in the driver)Acquisition
.demod_en_acq(False)
lengths
coincide withavg_cnt
Qblox #1088 (comment)Sweeps
Configs
.offset_awg_pathX()
, cf. binned acquisition tutorial.thresholded_acq_rotation()
and.thresholded_acq_threshold()
Notes
To be eventually converted into docs: