Skip to content
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

Draft
wants to merge 228 commits into
base: parse-q1asm
Choose a base branch
from
Draft

Qblox #1088

wants to merge 228 commits into from

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Nov 1, 2024

This will be the prototype for the 0.2 Qblox driver, based on the Q1ASM handling layed out in #868

Final checks

  • since the Pulse.id changed type, it should be kept compatible with QM and Qibocal Qblox #1088 (comment)
    • if not worth to keep as it is, rather revert to Pulse.id: int, keeping the attribute and UUID generation Qblox #1088 (comment)

Progress

Program

  • registers initialization
  • atomic pulse-like operations
  • mandatory play and acquire duration
    • (most likely) infer it from the waveform
  • long wait (> 2**16 ns)
    • keep count of available registers
    • ... though it may be sensible to reuse the same register
    • but the label has to be different
  • test Program serialization and deserialization (from JSON, not from Q1ASM)
  • merge padding into relaxation
  • split shots into batches, to fit large memory requirements

Sweeps

Configs

Acquisition

  • bin counter tracking
  • wait for completion
  • instrument acquisition (download data)
  • extract results
  • average over samples
    • by default the integration is just summing
  • acquisition type: distinguish discrimination, integration, scope
  • validate for raw (scope) acquisition limitations
    • at most 1 acquisition per channel per run
    • no loops (neither sweepers nor shots, it would be overwritten, not summed)
  • control demodulation for raw acquisition Raw acquisition and demodulation #1134
    • .demod_en_acq(False)
  • check if computed lengths coincide with avg_cnt Qblox #1088 (comment)

Notes

To be eventually converted into docs:

  • Qblox bin counter algorithm #1119
  • following the typical use, the sequencers' output on QCM is never modulated (used for flux pulses) while on all the other modules (i.e. QCM-RF, QRM-RF) they are always modulated
    • in principle, one may use a QCM in complex mode, with two outputs, and then further upconvert into the radiofrequency regime with an external LO

Further

  • long swept wait
    • sometimes the duration of a wait instruction may be swept (e.g. T1 routine), in which case it, and it may be longer than 65 us
    • if the range swept is smaller than 65 us, it is possible to still sweep a single wait instruction
    • otherwise, we can give up on the 1 ns resolution, and sweep multiple wait instructions at the same time: if the range is more than N*65k, but less than (N+1)*65k, I will sweep N+1 wait instructions in parallel (possibly in a loop), and have a resolution of (N+1) ns
      • this solution is limited in resolution, but simple enough
      • it is possible to use multiple registers for multiple wait instructions, and decide at runtime which one to increment (by using conditionals as jge), in order to never exceed the 65k for each register - but it's rather cumbersome, and having an N ns resolution should be acceptable for an ~N * 65k long wait instruction
  • unroll
  • implement acquisition weights (kernels)
  • baking
    • while Qblox has no limitation imposed by the time grid, which coincides with the sampling rate, there are still limitations on the minimal pulse duration, i.e. 4 ns = 4 samples for plain pulses, 8 samples for duration-swept pulses (4 * #instructions) - not a huge limitation, but we can overcome it through baking
    • some baking support may be lift to Qibolab itself (possibly parametrizing some device limitations)
  • mixer calibration

@alecandido alecandido added this to the Qibolab 0.2.2 milestone Nov 1, 2024
@alecandido alecandido self-assigned this Nov 1, 2024
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 17.95580% with 891 lines in your changes missing coverage. Please review.

Project coverage is 46.21%. Comparing base (8af95d1) to head (0ae4c2b).

Files with missing lines Patch % Lines
src/qibolab/_core/instruments/qblox/cluster.py 0.00% 133 Missing ⚠️
...bolab/_core/instruments/qblox/sequence/sweepers.py 0.00% 89 Missing ⚠️
src/qibolab/_core/instruments/qblox/config.py 0.00% 87 Missing ⚠️
...olab/_core/instruments/qblox/sequence/transpile.py 0.00% 68 Missing ⚠️
src/qibolab/_core/instruments/qblox/platform.py 0.00% 56 Missing ⚠️
...bolab/_core/instruments/qblox/sequence/sequence.py 0.00% 53 Missing ⚠️
.../qibolab/_core/instruments/qblox/sequence/loops.py 0.00% 49 Missing ⚠️
src/qibolab/_core/instruments/qblox/results.py 0.00% 47 Missing ⚠️
...lab/_core/instruments/qblox/sequence/experiment.py 0.00% 45 Missing ⚠️
...rc/qibolab/_core/instruments/qblox/mock/cluster.py 44.92% 38 Missing ⚠️
... and 15 more
Additional details and impacted files
@@               Coverage Diff               @@
##           parse-q1asm    #1088      +/-   ##
===============================================
- Coverage        55.90%   46.21%   -9.69%     
===============================================
  Files               73       96      +23     
  Lines             3456     4490    +1034     
===============================================
+ Hits              1932     2075     +143     
- Misses            1524     2415     +891     
Flag Coverage Δ
unittests 46.21% <17.95%> (-9.69%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alecandido
Copy link
Member Author

@stavros11 I start wondering why do we have this distinction at all:

device: str = ""
"""Name of the device."""
path: str = ""
"""Physical port addresss within the device."""

Couldn't we have just a path attribute, which is parsed by every instrument in its own way?

It would have made sense if Channel were only part of the Platform, and we had to attribute the instances to the various instruments, and thus device could have served that purpose, matching the name of the instrument in the Platform.instruments dictionary.
But Channels are already part of the Controller class itself, so we don't need to perform this operation at all...

Moreover, we internally distinguish channels only by their logical name (stemming from Qubit's attributes), while this physical layer is fully internal to the driver.

If we agree on this, what we could do is to deprecate .device, making it Optional and assigning a default None value.
Formally, this would break the interface (as there may be functions assuming that attribute to always be a string, and never None). In practice, it should not break anything.

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).

@stavros11
Copy link
Member

stavros11 commented Nov 3, 2024

Couldn't we have just a path attribute, which is parsed by every instrument in its own way?

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.

If we agree on this, what we could do is to deprecate .device, making it Optional and assigning a default None value. Formally, this would break the interface (as there may be functions assuming that attribute to always be a string, and never None). In practice, it should not break anything.

I think it will break the QM driver, as channel.device is used in a few places and using None there would probably lead to failure. It should be easy to update this to read the device from the path, however if we want to keep our promise and not break the public interface within 0.2 versions, we should also maintain the use of device for QM. Otherwise the current platforms of qibolab_platforms_qrc main which use device will fail until we change them. I should be able to do something like

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 device and use only path and in principle we are not breaking anything as these drivers never existed in the first place. Also, I think Keysight will not be affected by this.

@alecandido
Copy link
Member Author

Ah, yes, the idea was to absolutely do something like if channel.device is None, but the idea is that, in principle, even that would be breaking.

E.g. if you have a function that takes a Channel and does channel.device.startswith() that would be broken, since it relied on channel.device always being a string.

In practice, I don't expect that there is any function like that (nor anyone using Channel content beyond the drivers, which are internal).
However, let me take that proposal back: in this case, an empty string should be as good as None, and it's already there (even as default).

Comment on lines 14 to 15
def _eltype(el: str):
return "qubits" if el[0] == "q" else "couplers"
Copy link
Member

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.

Copy link
Member Author

@alecandido alecandido Nov 4, 2024

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.:

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.

Copy link
Member

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.

Copy link
Member Author

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:

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).

@alecandido
Copy link
Member Author

@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 io (aka both) option may be used at all...
I.e. the sequencer is attached to one or two physical channels, that are either input channels or output channels (obviously not both...). And even if it's attached to two channels, it's only to work in complex mode (I & Q).

Do you have any use case for this io mode? Is there anything evident I'm missing?
(I will also ask explanation to the support team, but I don't consider this urgent...)

@DavidSarlle
Copy link
Contributor

@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 io (aka both) option may be used at all... I.e. the sequencer is attached to one or two physical channels, that are either input channels or output channels (obviously not both...). And even if it's attached to two channels, it's only to work in complex mode (I & Q).

Do you have any use case for this io mode? Is there anything evident I'm missing? (I will also ask explanation to the support team, but I don't consider this urgent...)

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.

@alecandido
Copy link
Member Author

alecandido commented Dec 13, 2024

Expanded in #1088 (comment)

Update params during sweeps

  • resetting values after sweeping

  • strategy: process sweepers, collecting them in two dictionaries

    1. channels to sweepers (for those acting channel-wide)
    2. pulses to sweepers

    the first dictionary can be consumed in loop(), since it spans the whole channel (possibly at the end of the loop, as usual to ensure underflow avoidance), while the second one can be passed to play(), to wrap pulse instructions

@alecandido
Copy link
Member Author

alecandido commented Dec 13, 2024

Check ports assignment for microwave channels

Complex IO should require two ports each, but I'm always assigning a single one per channel.

(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')]
}

This is incorrect, and it is happening during channels' creation in the helper function used in the platform construction.

We do not use complex IO, since the mixing is happening internally.

alecandido added a commit to alecandido/qrc that referenced this pull request Dec 13, 2024
@alecandido
Copy link
Member Author

@DavidSarlle (cc @aorgazf), concerning our previous discussion about the io mode, I found out that we could actually make use of it, since we would use a single sequencer (aka processor) for input and output.

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).
Syncing the two operations to happen exactly at the same time may be slightly more complex with a single processor, since I should make sure that both operations are starting exactly at the same time[*]. But since it is not deadly relevant, both because of the finite time-of-flight and the option to acquire more and discard (even through kernels), it should be reasonable enough to execute a readout operation on a single sequencer.

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).
So, I currently listed it in my leftover issue #1091, and it won't be present as an option for the first iteration of this driver.

[*]: 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)

@DavidSarlle
Copy link
Contributor

@DavidSarlle (cc @aorgazf), concerning our previous discussion about the io mode, I found out that we could actually make use of it, since we would use a single sequencer (aka processor) for input and output.

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). Syncing the two operations to happen exactly at the same time may be slightly more complex with a single processor, since I should make sure that both operations are starting exactly at the same time[*]. But since it is not deadly relevant, both because of the finite time-of-flight and the option to acquire more and discard (even through kernels), it should be reasonable enough to execute a readout operation on a single sequencer.

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). So, I currently listed it in my leftover issue #1091, and it won't be present as an option for the first iteration of this driver.

[*]: 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)

@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.

@alecandido alecandido mentioned this pull request Feb 25, 2025
12 tasks
@alecandido
Copy link
Member Author

alecandido commented Mar 3, 2025

Minimum pulse duration sweep validation

Since an explicit wait instruction is required to control the duration of the pulse (play does not admit its duration to be set by a register), the two instructions together take a minimum of 8 ns.

As usual, this would result in an error in Qblox itself, since if less than 8 ns are desired, consequently the wait instruction will result in less than 4 ns, that is an error.
However, while during validation the error can be raised about the sweeper itself, if left to Qblox it will result in an error about a certain wait instruction, which should be connected to the duration sweeper it originated from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants