Acquisition

[alecandido]

Closes #916

• (de)serialize ChannelId
  • as qubit/ch_name and
  • as qubit/drive_cross/name only for cross-resonances channels
• fix Sequence.probe_pulses()
• define the Acquire pulse-like instruction
• replace probe Pulse parsing with Acquisition detection
• add helper to group subsequent probe Pulse and Acquire
• test the additions

[codecov]

Codecov Report

Attention: Patch coverage is 86.15385% with 18 lines in your changes missing coverage. Please review.

Project coverage is 42.67%. Comparing base (b49bb42) to head (1940ba2).
Report is 42 commits behind head on 0.2.

Files	Patch %	Lines
src/qibolab/instruments/qblox/controller.py	0.00%	5 Missing ⚠️
...rc/qibolab/instruments/emulator/pulse_simulator.py	0.00%	4 Missing ⚠️
src/qibolab/instruments/rfsoc/driver.py	0.00%	4 Missing ⚠️
src/qibolab/instruments/qblox/cluster_qrm_rf.py	0.00%	3 Missing ⚠️
src/qibolab/instruments/icarusqfpga.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              0.2     #970      +/-   ##
==========================================
+ Coverage   41.91%   42.67%   +0.76%     
==========================================
  Files          79       80       +1     
  Lines        5464     5537      +73     
==========================================
+ Hits         2290     2363      +73     
  Misses       3174     3174              

Flag	Coverage Δ
unittests	42.67% <86.15%> (+0.76%)	⬆️

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]

Analogously to #965 (comment), this may become handy:

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
    """
    new = []
    if seq is None:
        return None
    for ch, pulse in seq:
        if "amplitude" in pulse:
            pulse["kind"] = "pulse"
        elif "phase" in pulse:
            pulse["kind"] = "virtualz"
        else:
            pulse["kind"] = "delay"
        new.append((ch, pulse))
    if gate == "MZ":
        new.append(
            (
                ch.replace("probe", "acquisition"),
                {"kind": "acquisition", "duration": pulse["duration"]},
            )
        )
    return new


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

[alecandido]

Btw, just a fun fact: the Python validation in ChannelId slowed down the tests (because it is involved in all natives in dummy).

Now they are taking the double wrt to #971:

• https://github.com/qiboteam/qibolab/actions/runs/10423432743/job/28870128966?pr=971
• https://github.com/qiboteam/qibolab/actions/runs/10423161801/job/28869272928?pr=970

In practice, it's pretty much irrelevant. And the easiest way to speed up the tests is to stop testing the full product space of options in test_dummy (possibly fully killing it in favor of test_result_shapes).

However, it's a remarkable performance drop. I'm tempted to load sequences with channels as str, and only validate at a later stage, and only the natives we need.
But this should be done once per platform load, i.e. at most once per experiment. And making 1000 experiments, instead of batching them somehow, is already a major bottleneck. Still, I'm a bit disappointed...

[alecandido]

I will rebase asap, but other than that is ready.

[alecandido]

The Pulse.flux constructor now seems more a bug than a feature:

qibolab/src/qibolab/pulses/pulse.py

Lines 42 to 53 in 19dd810

	relative_phase: float = 0.0
	"""Relative phase of the pulse, in radians."""
	@classmethod
	def flux(cls, **kwargs):
	"""Construct a flux pulse.
	It provides a simplified syntax for the :cls:`Pulse` constructor, by applying
	suitable defaults.
	"""
	kwargs["relative_phase"] = 0
	return cls(**kwargs)

it's just overwriting .relative_phase, whatever the user has given. While a 0.0 default is already present, thus is sufficient to avoid specifying it...

Shall we remove it?

[alecandido]

Btw, I'm not fully convinced regarding names (e.g. as_readouts). If you have better suggestions, please propose :)

[stavros11]

If this duration is different than the corresponding probe, I am not sure how I would handle it in the instrument. At least for QM, the only way that I am aware to control this duration is the smearing, and maybe also the time of flight, however the latter works like a delay, rather than a different duration. However, we are already setting these parameters as part of the acquisition config, so wouldn't providing a different duration here duplicate them and potentially even create conflicts? For example a different smearing is needed to force the required acquisition duration, than the one provided in the config.

[alecandido]

The idea is to use it for instruments where they are truly independent, event per event.

In QM, you should use the _Readout object, generated by the PulseSequence.as_readouts property.
For the time being, I'm not validating that the probe and acquisition have the same duration, but I'm implicitly assuming so. I could even add an explicit validation in the _Readout object, or we can leave this up to the driver (either validate, or neglect one of the two).

[stavros11]

Using _Readout will ignore the duration of the probe pulse and use the acquisition one. This may not be ideal without throwing a warning/error if the two durations are different, to tell the users that we are not actually doing this. I have not tried, but it could be that an error will be raised by QM because the waveform will be generated using the probe, while the length (duration) will be read from the acquisition and if the two are not compatible the instrument will probably not like it. This may not be true for rectangular pulses, though, so it may be better to catch this earlier, either in as_readouts or in the driver.

[alecandido]

Warnings are pretty useless in practice (though we could think more about logging), they would simply be ignored. If someone notices a problem, there are high chances we'll be contacted before the user reading the warning...

We could raise manually, either in as_readouts/_Readout or in the driver. In the first case, it would be imposed to every as_readouts user. Though, it is likely that you want it, if you're using as_readouts.

[stavros11]

Generally, I have the impression that separating acquisition makes the life of everyone harder:

• qibolab developers have to support it in the drivers and maintain nasty functions like as_readouts,
• users (or routine authors) have to add more pulses in the sequence to achieve the same thing,
• platform authors have to add an additional pulse in the native measurement, essentially providing only an additional duration, that may even be ignored by some instruments.

and all this needs to be documented and well explained to users.

It is not clear to me what we get in return, other than the ability to acquire for different duration than the probe or potentially add some delay between the two, which was generally already possible with time of flight/smearing, and the ability to play acquisition without probe which is not clear which instruments can support it and if it is relevant for any practical applications, at least near term.

[alecandido]

In general, you could even acquire without any probe at all. This can be useful, not only for improving the measurement, but even for designing calibration experiments (e.g. for acquiring pulses in loop-back, and calibrating what it is the transmission baseline, and you would classify as 1 - but also to characterize physical units with external instruments, or among them).

[alecandido]

It may not be that useful during qubit operations (for which smearing/time of flight could be enough) but it may be strictly required for a different usage of the devices (i.e. their isolated calibration).

I hoped that smearing/time of flight were doing the exact same thing, just with a more convenient interface for experiments (as a rotation in the space of parameters). If there are instruments for which this is the case, so much the better, it's just a translation. Otherwise, they will fail whatever they can't do.
However, I see the acquisition as a more native operation (I'd argue it is), since the instrument DAC and ADC are different discrete elements, and there should be no bond that makes them operating at the same time. Other than the (programmable) logic built on top.

[alecandido]

In any case, if it's unbearable, we could even expose the Readout as part of the interface. If you don't need the split, you could use that. Possibly, with a simpler constructor (such that you don't have to specify both the events). But it doesn't seem that complicate, to me, defining the acquisition as a separate event (and possibly even avoid time of flight/smearing at all in Qibolab interface, compiling them within the driver).

Even better, but further in the future, we could avoid compiling the measurement completely, just keeping it as an atom in Qibolab representation (even in sequences), since most of the drivers will register the measurement parameters, and then just execute the measurement (which is exactly what smearing and time of flight are doing).

[stavros11]

The Pulse.flux constructor now seems more a bug than a feature:

qibolab/src/qibolab/pulses/pulse.py

Lines 42 to 53 in 19dd810

	relative_phase: float = 0.0
	"""Relative phase of the pulse, in radians."""
	@classmethod
	def flux(cls, **kwargs):
	"""Construct a flux pulse.
	It provides a simplified syntax for the :cls:`Pulse` constructor, by applying
	suitable defaults.
	"""
	kwargs["relative_phase"] = 0
	return cls(**kwargs)

it's just overwriting .relative_phase, whatever the user has given. While a 0.0 default is already present, thus is sufficient to avoid specifying it...
Shall we remove it?

Yes, is this used anywhere?

[alecandido]

Yes, is this used anywhere?

Only in the tests

[alecandido]

@stavros11 pay attention that these are more or less two features in one PR (related, but up to a point). One consists of the new channel identifiers, the other one of the automated readout collection.

[stavros11]

Isn't this a bit cyclic? We have, Qubit -> Channel -> ChannelId -> QubitId. Of course it is not cyclic in terms of objects because QubitId != Qubit, but at least conceptually wise it kind of is.

At this stage, I am not sure if we even need Qubit objects. We could just pass the logical channels directly to the platform and keep them in a dict[str, Channel]. The stricter would be dict[ChannelId, Channel], however that may be less user friendly when creating sequences, as

sequence.append((platform.channels["0/drive"], Pulse(...)))

is simpler than

sequence.append((platform.channels[ChannelId(...)], Pulse(...)))

even though we could also alleviate this by defining an appropriate getter in the Platform.

On the other hand, I am not sure if making channels depend on qubits (in this case QubitId) is a good idea. I understand here you are trying to standardize channel names. However, in general, channels are a more primitive idea than qubits, as in qubits need channels to be controlled, not the other way around. Channels could survive without qubits, potentially in non-quantum computing applications.

[alecandido]

Isn't this a bit cyclic? We have, Qubit -> Channel -> ChannelId -> QubitId. Of course it is not cyclic in terms of objects because QubitId != Qubit, but at least conceptually wise it kind of is.

The problem of your picture is Channel -> ChannelId: an object should not be aware of its own identifier, because that's how the other ones are calling you, but you don't really need to call yourself.

On the other hand, I am not sure if making channels depend on qubits (in this case QubitId) is a good idea. I understand here you are trying to standardize channel names. However, in general, channels are a more primitive idea than qubits, as in qubits need channels to be controlled, not the other way around. Channels could survive without qubits, potentially in non-quantum computing applications.

True, but Qibolab is mostly about quantum computing.

In any case, the main reason why ChannelId now contains QubitId is exactly because Qubit contains the Channel: if you have a pulse sequence, you truly want to be able to get a handle on the channel, given a platform. So, the ChannelId is a practical path to retrieve the Channel you need.
If instead we give up on Qubit as a Channel collection (and thus on the Qubit object tout-court, because that's what is left), we don't need to navigate, but just grab the channel from the channels' container (a single platform attribute).

[alecandido]

The stricter would be dict[ChannelId, Channel], however that may be less user friendly when creating sequences, as

sequence.append((platform.channels["0/drive"], Pulse(...)))

is simpler than

sequence.append((platform.channels[ChannelId(...)], Pulse(...)))

even though we could also alleviate this by defining an appropriate getter in the Platform.

Here you're a bit unfair, since the current layout is just:

sequence.append((ChannelId(...), Pulse(...)))

And that's better than everything: a pulse sequence should not be bound to a specific platform, but just a sequence of operations. If you change platform, but you keep the same channel IDs, the instructions should still execute (which is relevant for serialization, when you dump and load the sequence again).

In any case, without the Qubit, we could definitely also have a simpler ChannelId, just reminiscent of the role of the channel (i.e. the ChannelType)[*], while retrieving the qubit association from somewhere else.

In a sense, this is really a leftover: by now, everything is working by named reference, without nesting objects (other than in parameters.json), and that was the idea of the components configs. But we could treat the actual components in the same way, and just retain the name in the Qubits (to answer the question: which is the channel for this qubit's flux?)

[*]: we could drop even the ChannelType, but this information is instead relevant even in the PulseSequence, so I'd try to keep it somehow - the other option is to make PulseSequence a full-blown object, not just a list, and register the channel's roles in there (not sure about which could be an effective interface)

[stavros11]

The problem of your picture is Channel -> ChannelId: an object should not be aware of its own identifier, because that's how the other ones are calling you, but you don't really need to call yourself.

But isn't the Channel aware ChannelId in this PR? Channel.name is the ChannelId. The -> in my original message basically means "has a reference to". Qubit has references to Channels, Channel has reference to its ChannelId, etc..

In any case, the main reason why ChannelId now contains QubitId is exactly because Qubit contains the Channel: if you have a pulse sequence, you truly want to be able to get a handle on the channel, given a platform. So, the ChannelId is a practical path to retrieve the Channel you need.

I think I don't understand this. How ChannelId helps retrieving Channels? I think the way to retrieve them as it is now is still platform.qubits[qubit_name].drive, etc. but that was working even before this PR.

If instead we give up on Qubit as a Channel collection (and thus on the Qubit object tout-court, because that's what is left), we don't need to navigate, but just grab the channel from the channels' container (a single platform attribute).

Yes, that's basically my proposal.

[alecandido]

But isn't the Channel aware ChannelId in this PR? Channel.name is the ChannelId. The -> in my original message basically means "has a reference to". Qubit has references to Channels, Channel has reference to its ChannelId, etc..

Yes, it was not a mistake in your reconstruction, just what I see as problematic.

I think I don't understand this. How ChannelId helps retrieving Channels? I think the way to retrieve them as it is now is still platform.qubits[qubit_name].drive, etc. but that was working even before this PR.

See below #970 (comment)

In principle, just reading the PulseSequence you didn't know it was the drive (because names were just whatever str). So, you had first to find (somehow) that your channel was referred to that qubit, and it was drive. Then you could access.
In practice, you were doing this by keeping a copy

qibolab/src/qibolab/instruments/qm/controller.py

Line 140 in b49bb42

channels: dict[str, QmChannel]

https://github.com/qiboteam/qibolab_platforms_qrc/blob/4a5bb22dff21ffee0bc2ffca8e136a42003dfda4/qw11qD/platform.py#L71
and never accessing the channels in the Qubit objects

qibolab/src/qibolab/platform/platform.py

Line 103 in b49bb42

qubits: QubitMap

[stavros11]

In the previous comments I am mainly talking about how users (mostly qibocal) are supposed to access channels and create sequences, not the driver.

The driver certainly cannot access channels from Qubits because it does not have access to Qubits and it shouldn't. But the driver also does not care whether a channel is a drive of a particular qubit or something else. It only cares what are the pulses playing on that channel (provided by the sequence) and what is the config (frequencies, offsets, potentially LOs - provided by the configs dict). It may only need to distinguish acquisition channels from the rest, but I think even this is not necessary now since there is a separate Acquisition instruction.

[alecandido]

Correct. But the driver should also be the only one to care about the Channel object. So, it makes much more sense to locate the Channel in the instrument, rather than in the Qubit in the Platform.

[stavros11]

Following the previous comment, this seems like duplicating Qubit, that's why I am not sure if we need Qubit in the end.

[alecandido]

As explained above, it is, exactly because we need to navigate the Qubit object to access the channel.

You can consider the ChannelId qubit/chtype to be the "path":

getattr(platform.qubits[qubit], chtype)

[stavros11]

I guess here we are forcing channel names to be f"{qubit_name}/{channel_type}". Except the concern in the previous comment (whether channels should depend on qubits) and the fact that it is a bit ZI inspired, it is not necessary bad. The main thing I don't fully like is that we are sort of programming via strings and this results to the usual ugly parser (see PulseShape and eval in 0.1, just different kind here).

I guess the proper solution would be to initialize these explicitly

IqChannel(name=ChannelId(0, ChannelType.DRIVE))

however, we still a str representation to match with the configs in the JSON, so we would probably end up in the existing one.

[alecandido]

The main thing I don't fully like is that we are sort of programming via strings and this results to the usual ugly parser (see PulseShape and eval in 0.1, just different kind here).

They are connected because both related to (de)serialization, but PulseShape was a much more structured object (a full pulse), while this is pretty simpler (the complicate part is that they are 2 or 3 sections, and that's variable - without drive_cross it would be much easier).

However, I also don't like it too much, because I'm "manually" parsing, while it could be done automatically by Pydantic (which, unless there are Python functions in the middle, is generating a JSON schema, and then processing the JSON in Rust).

To be fair, drive_cross is also a bit silly, because is not a channel associated to a single qubit, but more to a pair...
(conceptually, then it will play on a physical line, but it's relevant up to a point whether it is connected to one qubit or the other, it's just addressing the pair)

I'd like much more to have chtype/something, where something may contain the qubit, but also a qubit pair and what not (qudits, if we go beyond qutrits, and whatever else).
As said above, the association channel <-> qubit could be kept separate from the channel retrieval.
Moreover, chtype doesn't even to be that granular, but it's just convenient to have the same information encoded by the channel classes locally in the sequence, i.e. separate DC, modulated, and acquisition.

[stavros11]

As said above, the association channel <-> qubit could be kept separate from the channel retrieval.

I also don't understand this. To me it seems that now we cannot even initialize a Channel without refering to a specific QubitId.

[alecandido]

Assume that we have a single Platform.channels container (even better, a Instrument.channels container, and Platform.channels just being the union of them), we wouldn't need Channel.name. The name would be just the key in the dictionary, and you only need the ChannelId to access both:

• the configurations
• the object required to play the pulse

So, there is no need to go from Channel to ChannelConfig, as you will always go from ChannelId to both of them.

[alecandido]

In #1001 I'll try to go back to plain strings, without any special meaning.

[stavros11]

Another point to note is that currently we have the .acquisition attribute in the probe channel and the .probe attribute in the acquisition channel to link one to the other. If you see for example in

qibolab/src/qibolab/components/channels.py

Line 52 in 1940ba2

FIXME: This is temporary solution to be able to generate acquisition commands on correct channel in drivers,

these were temporary solutions until the acquisition business is sorted (done in this PR).

These attributes ended up being quite handy for the QM driver, because we need to map both these channels to the same element in the QM config and having the immediate connection through attributes makes it easier. However, the addition of ChannelId makes them redundant, since now we can establish the same connection by finding the qubit q = probe.name.qubit and then looking for f"{q}/acquisition" in the global channel map. This would require updating the QM driver once again, but that's acceptable as we are finalizing the design.

As said before, I am not sure if it is optimal navigating through channels using the qubit name, however the previous design was not flawless either (it already had the assumption that a probe should have an acquisition), so it should be fine to switch.

[alecandido]

I'm actually inclined to merge this PR as is, but add another one on top of the stack, where I'll try to implement some alternative proposals mentioned here, starting from an attempt to remove the Qubit object.

The other option is that I'll rebase the stack directly on 0.2, and keep this separate. However, ChannelIds are used a bit in #992, so it's slightly simpler to do it on top (though I will do it on top of #994, not #995, which is unfinished).

[stavros11]

Following the last comment, I guess it is convenient to merge this and potentially improve with further PRs on top. @alecandido I am not sure if you would like to open issues with your plan, but if you are already working on it, even just a PR with a checklist could be sufficient.

[alecandido]

@stavros11 now all points raised in this PR should be present as items in #1001, and I'll try to simplify the things there, taking a more global perspective (not just fixing some channel accesses or a pulse sequence hacky method, but all at once). It was handy to this in steps, since it was not a simple problem, but a final look should help to find a better "global minimum" (now that we're closer to its region).

If you find something that I missed from the discussions, please add it to #1001 OP.

I'm merging this.