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 105 commits into
base: parse-q1asm
Choose a base branch
from
Draft

Qblox #1088

wants to merge 105 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

Progress

  • registers initialization
  • atomic pulse-like operations
  • looping machinery
  • bin counter tracking
  • check ports assignment for microwave channels Qblox #1088 (comment)
  • test Program serialization and deserialization (from JSON, not from Q1ASM)
  • find new name for Sequence
    • it's potentially shadowing Python's collection Sequence (which I'm also using in the driver)
  • unroll

Acquisition

  • 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
    • e.g. at most 1 acquisition per channel per run
  • 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)

Sweeps

Configs

  • handle offset
  • set LO frequency
  • set NCO frequencies
  • set classification parameters
    • Qcodes, .thresholded_acq_rotation() and .thresholded_acq_threshold()

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

@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 19.66942% with 486 lines in your changes missing coverage. Please review.

Project coverage is 48.52%. Comparing base (5c73d4f) to head (15cafdc).

Files with missing lines Patch % Lines
src/qibolab/_core/instruments/qblox/cluster.py 0.00% 91 Missing ⚠️
...ibolab/_core/instruments/qblox/sequence/program.py 0.00% 65 Missing ⚠️
src/qibolab/_core/instruments/qblox/config.py 0.00% 63 Missing ⚠️
...bolab/_core/instruments/qblox/sequence/sequence.py 0.00% 50 Missing ⚠️
src/qibolab/_core/instruments/qblox/platform.py 0.00% 41 Missing ⚠️
src/qibolab/_core/instruments/qblox/results.py 0.00% 39 Missing ⚠️
src/qibolab/_core/instruments/qblox/log.py 0.00% 36 Missing ⚠️
...rc/qibolab/_core/instruments/qblox/mock/cluster.py 42.62% 35 Missing ⚠️
...olab/_core/instruments/qblox/sequence/waveforms.py 0.00% 23 Missing ⚠️
...ab/_core/instruments/qblox/sequence/acquisition.py 0.00% 14 Missing ⚠️
... and 6 more
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     
Flag Coverage Δ
unittests 48.52% <19.66%> (-5.84%) ⬇️

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.

@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

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
Copy link
Member Author

lengths in

def extract(
acquisitions: dict[ChannelId, AcquiredData],
lengths: dict[acquisition.MeasureId, int],
) -> dict[PulseId, Result]:
return {
int(acq): _extract(idata["acquisition"]["bins"]["integration"], lengths[acq])
for data in acquisitions.values()
for acq, idata in data.items()
}

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
https://docs.qblox.com/en/main/tutorials/q1asm_tutorials/basic/generated/QRM/020_binned_acquisition.html#Retrieve-acquisition
(cf. module.sequencer0.integration_length_acq()).

However, the collected mapping which is retrieved by module.get_acquisitions(0) actually contains an avg_cnt field, which may correspond to the same information, and even more refined (since it will be per-bin, allowing for non-uniform bin filling - which should not happen anyhow in a Qibolab-generated execution).
If that's the case, it's hard to justify the other approach, and it will be a considerable simplification to just resort to the information obtained from .get_acquisitions().

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