Channel api #885

hay-k · 2024-04-26T06:05:19Z

Main highlights:

Concepts of component, channel, and component config are introduced (see respective docstrings for more detail)
Pulse sequence is a dict now
All native gates are pulse sequences, plus there is a new concept of pulse sequence factories (which is implemented as minimal as possible for, but still covering all existing use cases).
Unrolling in platform level does not stitch pulse sequences together into one huge sequence - it passes them to the driver as a list, and driver is responsible to figure out what is the best way to execute them. E.g. for ZI driver this reduces a lot of unnecessary complication and even makes the schedules more precise. For other drivers even if it does not bring any benefit the stitching can be done inside the driver itself (it is just a call to one function)
Got rid of all qubit and coupler references in the ZI driver (same should be done for all drivers).
ZI driver is adapted according to the channel API.
Zi driver is adapted to other (not related to channels) changes in 0.2

NOTE: The pipeline is failing because of examples in the docs, which I will fix soon. Plus I am expecting to fix some outdated docstrings and maybe some minor changes in the ZI driver (still need to test it against actual quantum computer). Other than this should be ready for final review.

tests/dummy_qrc/qblox/platform.py

alecandido

I still have to review what has been implemented inside ZI, but the rest is pretty minimal and definitely an improvement.

So, as soon as it will be finalized, we could even merge more or less as it is, and postpone further improvements (further Qubit's attributes removal, Pulse.qubit and Pulse.channel removal, Couplers replacements) to further PRs.

src/qibolab/channel_configs.py

src/qibolab/channel.py

src/qibolab/channel_configs.py

src/qibolab/pulses/sequence.py

src/qibolab/platform.py

src/qibolab/dummy/platform.py

stavros11

I agree with the major changes introduced here: the channels/configs and simplification of PulseSequence. I will give a try to update the QM driver based on this and see if there are any issues.

Other than that there are at least two things that I think should be added here (or in a follow up PR):

Examples about how to write platforms, particularly to show how the channel creation and instrument settings will be handled. We should start with the platforms in tests/dummy_qrc and propagate to the real platforms repo later.
Drop pulse.qubit and pulse.channel. These are certainly not needed with the new PulseSequence structure. Of course we won't update all drivers for this in this PR, this will be done incrementally, but we should at least drop from the Pulse object and any other references in the interface.

Then there is a debate about pulse.type. In principle it is redundant since the channel is sufficient to define the type, however PulseSequence only has the channel name which is not sufficient to infer the type, so I am not sure if we can easily drop it with the current design.

src/qibolab/qubits.py

src/qibolab/pulses/sequence.py

src/qibolab/channel.py

hay-k · 2024-04-30T08:21:35Z

Examples about how to write platforms

I would probably want to do this as separate PR.

Drop pulse.qubit and pulse.channel

This PR.

Then there is a debate about pulse.type

I am of the opinion that this should be removed, but this coin has two faces. One face is the channels, which no longer needs to be handled by this property. The other face is that this is now used in a lot of places, especially for identifying RO pulses, which is kinda irrelevant to channels, but still should be done in other ways. So I think we can remove this a bit later, once those things are cleaned up.

src/qibolab/channel/channel_map.py

src/qibolab/platform.py

alecandido · 2024-04-30T13:45:14Z

src/qibolab/pulses/sequence.py

-        synchronize channels + other."""
-        # TODO: implement
-        ...
+    def append(self, other: "PulseSequence") -> None:


Why did you decide to avoid the dunder?

In any case, if you want a name, I'd suggest:

Suggested change

def append(self, other: "PulseSequence") -> None:

def extend(self, other: "PulseSequence") -> None:

because the syntax (and semantic) of list.append() is list -> element -> list, while list.extend() is list -> list -> list (ok, not returning, but mutating in-place, but the meaning is that).

This has been renamed to extend. The reason I decided not to go with the dunder method is that PulseSequence object is by design a highly mutable object, and main use case would be to modify it in place anyways, so ps2 = ps0 + ps1 is not predicted to be a common use case

But if __add__ is present, you can always use as:

ps0 += ps1

which is basically .extend().

You can also use it as c = a + b, while with extend you can't, which is more clear interface.

Well, c = a + b is the exact same interface of lists, as also .extend() is. Since you're mimicking a list, it doesn't sound unreasonable to support its basic operations.

You could even implement .__add__() and .__iadd__() on top of .extend().

alecandido · 2024-04-30T13:49:18Z

src/qibolab/pulses/sequence.py

+    def append(self, other: "PulseSequence") -> None:
+        """Appends other in-place such that the result is self + necessary
+        delays to synchronize channels + other."""
+        tol = 1e-12


Top-level constant?

Well, it is used only here, I don't see any reason to move it to top level.

Avoid hiding constants. If nothing else, it will be documented.

It is not hidden, it is right there. I can mention it in the docstring though.

What is your motivation to actively keep this outside top-level?

As a constant, not only it would be documented, but other functions could also make use of it (both outside Qibolab, inside Qibolab, or even for testing). In the docstring it would be only human-readable, and then possibly copied when needed.

What is your motivation to actively keep this outside top-level?

I don't see the point. A lot of things can be generalized, overengineered, and made future-proof or future-usable for a future that is not known/

but other functions could also make use of it

This is exactly what I want to avoid - people accidentally using things that they shouldn't. If you want to introduce a global system for handling numerical imprecisions that is suitable for any context, maybe it can be discussed outside of this PR.

I don't care specifically about numerical accuracy. Nor I want any specific use of this value.
I'm not proposing anything specific to this instance of the problem, but just a general guideline: avoid hard-coding values. Being them numbers or strings.

If you need values in your code that are not part of the user input, give them a name and document them properly.

Moreover, there is no way this request corresponds to overengineering: you would use the exact same thing (you're assigning a name anyhow), without any further feature. The only plain addition would be the explicit docs.
And yes, you could avoid documenting internals. But we decided not to do that (many people are working on the projects, so internals' documentation provides benefits for further contributors).

alecandido · 2024-04-30T13:52:03Z

src/qibolab/pulses/sequence.py

+        durations = {ch: self.channel_duration(ch) for ch in other}
+        max_duration = max(durations.values(), default=0.0)
+        for ch, duration in durations.items():
+            if delay := round(max_duration - duration, int(1 / tol)) > 0:


I like that is very concise, but side-effects are a bit implicit, if possible I'd avoid.

Moreover, unless you are very familiar with :=, the priority wrt > is not incredibly clear.

I'd just go for something much more basic, and for sure more verbose, but completely equivalent:

Suggested change

if delay := round(max_duration - duration, int(1 / tol)) > 0:

delay = round(max_duration - duration, int(1 / tol))

if delay > 0:

alecandido · 2024-05-03T14:19:28Z

I added a few fixes, and in particular, in 7660a67, I dropped the PortType and ports from the Controller object, since that is not the current mechanism in this proposal.

However, the actual instruments (including dummy) are still implementing that, thus their tests are failing.
But I didn't want to touch, since we will need a drivers update anyhow, and it doesn't make sense to destroy the current tasks performed, since they will simply be rephrased under the new abstractions.

stavros11 · 2024-05-04T12:09:00Z

However, the actual instruments (including dummy) are still implementing that, thus their tests are failing. But I didn't want to touch, since we will need a drivers update anyhow, and it doesn't make sense to destroy the current tasks performed, since they will simply be rephrased under the new abstractions.

We should still fix the tests used by the CI before merging this to have some basic testing on for the 0.2, but I think you already know that as we have been doing it so far. We don't need to fully update drivers for this, as the instrument tests are skipped, maybe only remove or update some imports if pylint complains. I would still fix the dummy though, as it shouldn't be that much work.

alecandido · 2024-05-04T18:22:40Z

It shouldn't be much work in general, I simply skipped it since I'm not sure at which state of completion is the PR (I'm not in @hay-k's mind).

I'd also like to have the tests fixed, not just because we have done it so far, but I'll use them to lead the implementation of #891

src/qibolab/channel/__init__.py

src/qibolab/serialize.py

alecandido

Minor comments.

But: would it be possible to start fixing the tests? Even at a shallow level (just passing, though not the best tests/coverage yet, or even just making possible their collection, by fixing the import errors...)

src/qibolab/channel/__init__.py

alecandido · 2024-05-14T13:05:43Z

src/qibolab/platform.py

@@ -167,14 +167,14 @@ def disconnect(self):
                instrument.disconnect()
        self.is_connected = False

-    def _execute(self, sequence, channel_cfg, options, **kwargs):
+    def _execute(self, sequences, channel_cfg, options, **kwargs):


This is already addressed in #861, and, if possible, I still believe it's a good idea to keep the various features in different PRs (as much as possible).

Is it a required upgrade here?

This change is independent from the unification effort. This is because I have removed the unrolling. Now after splitting into batches platform submits each batch as a list to the instrument, instead of unrolling them into a single sequence. If particular instrument needs unrolling, the function is still there, they can call the unrolling function themselves. This change was introduced to simplify the ZI driver, which was previously trying very hard to un-unroll the sequences back.

This is completely ignoring the idea of unrolling, which is controlled by the Qibolab user (within all the limits of the device used), not by the driver developer.

Please, show an example for the ZI driver, and we can improve on that side, instead of removing the feature for everyone else.

Since instruments should be capable of executing arbitrary sequences (within their memory limitations) unrolled sequences should be perfectly legitimate and indistinguishable from similar sequences requested by the user. If ZI is not capable of executing them, then we have a problem with the ZI driver.

I finally managed to find this discussion!

To be fair, the current implementation of unrolling is quite useless for QM as well, due to various issues previously discussed in other places (not being able to bypass the compiler, etc.). Maybe "useless" is strong word, it could still speed up a few routines, but most likely far from optimal.

Regardless, I do not see any issue with the change implemented here. The batching, which is a bit more complicated code-wise, is still done at the platform level. The only overhead is calling unroll_sequences in every driver, but this is just a single line of code. Having the list of sequences in the driver is even more flexible if the driver developer does not want to naively use unroll_sequences for some reason, for example if the underlying instrument provides a better way to unroll.

I would certainly not block the PR for this. It is also not interface facing, it can be easily changed in the future if needed without affecting other repos and users.

I agree the case of QM is peculiar, because of the compiler.
However, even that doesn't change the message: unrolling is an optimization feature, if we're unable to optimize for QM, you should simply not use that.

Same story for Zurich: if you can't use it, don't do it.
Though I have the feeling that for Zurich the problem is deeper, since it had to "un-unroll", which it should not be required at all, since the unrolled sequence is just a sequence.

Calling unroll_sequences in all drivers is not just overhead, but a design flow. Drivers should be able to play arbitrary sequences (bound to hardware limitations), and they should not distinguish between an unrolled one and not unrolled one.

Having the list of sequences in the driver is even more flexible if the driver developer does not want to naively use unroll_sequences for some reason, for example if the underlying instrument provides a better way to unroll.

This we should carefully discuss. And always avoid in the first approximation: passing more information down to the drivers by default means overhead for each and every new driver.

The goal of Qibolab is to abstract features of the various instruments, in order to make it simple for users (first Qibocal) to execute experiments on different devices.
Requiring the drivers to do more makes it heavier and more difficult to maintain many drivers, and we have at least 6 platforms to take into account (more to come, at least other 2 in the short/middle term), not just ZI and QM.
We should struggle to keep the interface simple, and make the effort of abstracting devices' details away.

If Zurich can not work with an unrolled sequence, I would have liked a dedicated issue and discussion. The implementation after, and in a different PR.

I would certainly not block the PR for this. It is also not interface facing, it can be easily changed in the future if needed without affecting other repos and users.

I'm going to accept many things I don't like in this PR. But if you don't want to wait for this one, I volunteer to revert it myself. At the price of breaking ZI (which it could have been fixed in a different PR anyhow).

This is completely ignoring the idea of unrolling, which is controlled by the Qibolab user

How is it controlled by user? As far as I know, qibocal does the following: 1. if user specified the unrolling option, it uses the execute_pulse_sequences method, to submit the list of pulse sequences, 2. if unrolling option is not specified, qibocal uses either sweep or execute_pulse_sequence method to submit a single sequence. From user's perspective they are either submitting a list of sequences, or a single sequence. After the unification of all these execution functions, there is one execute method only that accepts a list of sequences. If user wants to submit their sequences one by one, they can submit lists of sequences of length one, if they want to submit all, they submit all. They only know, that submitting all may bring some performance benefits, but how exactly, does not matter, it is internal implementation detail. With my change, from the perspective of users experience changes exactly nothing.

I believe some details are misunderstood about this change. By removing the call to the unrolling function before execution is submitted to instrument, does not mean performance optimization brought by unrolling are gone for the zi driver. It still executes the entire batch as one, but it is free from the structure enforced by the unrolling in platform level. You could say "the driver does unrolling its own way".

Calling unroll_sequences in all drivers is not just overhead, but a design flow. Drivers should be able to play arbitrary sequences (bound to hardware limitations), and they should not distinguish between an unrolled one and not unrolled one.

Sure, each driver should be able to execute arbitrary sequence. I don't see how moving the place where unroll_sequences is called (or not called) changes this. In case of zi driver specifically, there is a possibility to do more optimized and precise execution if you know that you are executing list of independent experiments as compared to a single big experiment, but you are still able to correctly execute single big experiment as well for sure.

Requiring the drivers to do more makes it heavier and more difficult to maintain many drivers

I mean, this claim taken out of context and in general is true. If you can reliably generalize some operations into platform, or wherever, instead of repeating in each driver, you should certainly do it. However it has negligible overlap with the discussion in this thread, which is just about moving the place of a function call.

Sure, each driver should be able to execute arbitrary sequence. I don't see how moving the place where unroll_sequences is called (or not called) changes this. In case of zi driver specifically, there is a possibility to do more optimized and precise execution if you know that you are executing list of independent experiments as compared to a single big experiment, but you are still able to correctly execute single big experiment as well for sure.

Let's focus on this specific point: why independent experiments are better supported than a long one?

alecandido

Still missing a few relevant files

alecandido · 2024-06-20T09:35:18Z

src/qibolab/components/__init__.py

+"""Component (a.k.a. logical component) is a concept that is part of the
+qibolab interface exposed to its users.


Suggested change

"""Component (a.k.a. logical component) is a concept that is part of the

qibolab interface exposed to its users.

"""A component of the user interface (a.k.a. logical component).

Try to keep it short, such that it fits a single line (we have a lint for that).

why though? :D

https://peps.python.org/pep-0257/#multi-line-docstrings

alecandido · 2024-06-23T07:47:10Z

src/qibolab/components/channels.py

+
+
+@dataclass(frozen=True)
+class IqChannel(Channel):


Just because we can, I'd choose one between *Channel and *Config and remove the suffix (i.e. we could have Iq and IqConfig).

Their identity is not given by their name in any case, since one is identified as a subclass of Channel, the other as part of the Config union.

Their identity is not, but readability is.

Well, Iq is not much less explicating than IqChannel, and the context should make it pretty clear it is a channel (other than its parent class).

src/qibolab/execution_parameters.py

alecandido · 2024-06-23T07:53:21Z

src/qibolab/instruments/zhinst/components/channel.py

+    device: str
+    """Name of the device."""
+    path: str
+    """Path of the device node."""


Couldn't we try to standardize this across all instruments?

I guess we will need a similar piece of information, one way or another (this was part of the role of Ports). I.e. a physical address on the machine (unless we can dub with the channel name, and convert internally).

In any case, I believe that this ZiChannel is in no way specific to ZI. Or better, the exact address representation could be specific, but we could have an Address object, and its driver-specific subclasses, without the need of a Channel wrapper (to handle more directly channels in the drives, that could allow us to more easily deduplicate some operations across drivers).

Of course, there is some kind of trade-off between making everything driver-specific (duplicating) and everything driver-agnostic (meaning that we might have to translate a lot, developing arbitrarily convoluted encoding of specific information in common classes).

My belief is that this could avoid involving channels, other than a minimal part, that will be paralleled, though not the exact same. But it is more or less subjective.

@stavros11, any opinion related to QM?

I this is exactly what was done by the plethora of Port classes in the past, and I think it didn't work out that well. The main idea in this proposal is that each instrument defines their own channels however they want, without any restriction. Instrument channels are not supposed to be exposed to users - only respective driver knows what it is, so this should be fine.

Well, the plethora of Port classes is going to be replaced by a swarm of Channels.

What I'm proposing above is to avoid any driver specific object (so it can not be the same).
If the channels are not user facing, they are not going to exist in the public API, so the information is being conveyed in some other way. And if the user has enough room to pass this information without them, we should be able to deal with everything without them.

Essentially, we already have a group of driver specific objects, the Instruments. So, we should be able to pass all the information that is specific to them in their configurations, and avoid any further specialized object.

I think having a specialized channel for each instrument is still useful. For example for QM

qibolab/src/qibolab/instruments/qm/components/channel.py

Line 11 in 443dac8

class QmChannel:

, I introduced a channel that is a bit different than the ZI counterpart, since we are using device name (str) and port (int) as identifiers, in contrast to the ZI path.

I think the price of separate channels is that the users writing platforms (ie. qibolab_platforms_qrc) need to be familiar with the channel API for the instruments they are using, which is a fair assumption. We could probably propose a unified interface that is the same for all instruments and do the translations internally in qibolab, however this would increase the code complexity and we would not get that much in return.

I believe the instrument specific channels are user facing

That was my understanding, but not what @hay-k wrote above:

Instrument channels are not supposed to be exposed to users

That's why I was arguing.

We could probably propose a unified interface that is the same for all instruments and do the translations internally in qibolab, however this would increase the code complexity and we would not get that much in return.

About this I do not fully agree: instead of the device-specific class, we would have a device-specific conversion, of objects that should somehow be similar.
The tradeoff is set by how similar are the objects, and how much information should be encoded in there. Maybe we could just discuss the alternatives (even live), also because this is the current difference I see:

@dataclass(frozen=True) class QmChannel: """Channel for Zurich Instruments (ZI) devices.""" logical_channel: Channel """Corresponding logical channel.""" device: str """Name of the device.""" port: int """Number of port.""" @dataclass(frozen=True) class ZiChannel: """Channel for Zurich Instruments (ZI) devices.""" logical_channel: Channel """Corresponding logical channel.""" device: str """Name of the device.""" path: str """Path of the device node."""

that to me seems the exact same code, just that in one case the port is an integer, and in the other there is a more generic path (which you can easily consider as a superset of integers, through the trivial int() encoding).

(btw, they are so close to each other that the docstring says ZI for both :P)

Well, the plethora of Port classes is going to be replaced by a swarm of Channels.

With ports it was different. There was an attempt to have a unified handling of them, hence there was an attempt to introduce a parent class Port, but all child classes had nothing to do with the parent. With device specific channels there is no attempt to generalize anything.

I believe the instrument specific channels are user facing, as they are needed to define platforms

User: person who runs calibration experiments, or quantum algorithms. What user does is repetitive and never ending activity.

Administrator / User level God: person(s) who sets up the platform so that users can use it. What administrator does is done once and may need occasional adjustment over time.

In some cases the two are the same person, in which case it is their responsibility to know knowledge about both worlds. Users calibrating or running quantum circuits do not need to know platform details, hence those are not exposed to them. Administrator needs to know every single detail, hence instrument specific channels are for them.

Well, I see your point in splitting the two kind of users. But my definition was easier (and I still believe the most common one), i.e. whoever is invoking Qibolab functions outside Qibolab itself.

In any case, it is not much relevant to discuss the user word definition.
Instead, the substance is that Qibolab development is a separate process from Qibolab usage, and the distinction is about what is part of the public API and what not.
Someone writing a platform has to know the details about the instrument. But it doesn't have to know anything about Qibolab's code, other than what is explained in the official docs.
Having a single channel means that it's easier to move from one instrument to another, and it would avoid device-specific docs for Qibolab. If possible, it's simpler in many respects.

What I'm proposing here is just to replace the two classes above with:

@dataclass(frozen=True) class Channel: logical_channel: Channel """Corresponding logical channel.""" device: str """Name of the device.""" path: str """Internal address."""

and to interpret within QM path as the port number with int(ch.path).

I don't see any further information that should belong to one driver and not the other. And even in the case in which there would be something like that, related to a feature truly available on one device only (not just a different encoding), we could keep spare attributes, until they are a minority (for the time being is this minority amounts to zero).

Well, I see your point in splitting the two kind of users. But my definition was easier (and I still believe the most common one), i.e. whoever is invoking Qibolab functions outside Qibolab itself.

I also have this definition in mind. Ideally, if Qibolab is simple enough, documented and more importantly stable, we shouldn't need administrator level users to write the platforms. People in the lab connecting them (= standard users) should be able to do it themselves.

What I'm proposing here is just to replace the two classes above with:

@dataclass(frozen=True) class Channel: logical_channel: Channel """Corresponding logical channel.""" device: str """Name of the device.""" path: str """Internal address."""

and to interpret within QM path as the port number with int(ch.path).

This sounds like a reasonable suggestion. It would certainly work for ZI and QM. I am not sure about other instruments, but I would expect it would, since str can be easily used to represent different things. I believe we could give a try having a single Channel, it would also simplify writing platforms hopefully making it more accessible to (non-administrator) users. We can still keep the Configs instrument specific.

alecandido · 2024-06-23T07:58:04Z

src/qibolab/instruments/zhinst/components/configs.py

As discussed, it would be nice to have a common setting even for the power, since it can be rather common. Even with a driver-specific interpretation of the numerical value.

However, the documentation is relevant, and it should be preserved.

Since this is outdated, I am not sure what it refers to, but I am assuming it was about gain, attenuation, and power_range. I would prefer to keep unifying them out of this PR, since it does not seem trivial and will need proper research and planning. For now instrument specific component configs are supposed to implement their setting, similar to what I have implemented for zi

Yes, it was referring to gain, attenuation, and power range.

This PR is already breaking channels for good, and not updating all the drivers (that is going to be done anyhow in separate PRs, like #874), so I'd say it's the ideal place for this.

What I'm proposing is pretty simple: drop the whole file, and a single attribute (called power, or whatever you prefer) in the general channels.
This attribute will be a float, as it is here, and it will be interpreted differently by each driver, despite being the same attribute.

Notice that, other than the documentation, here you're just adding another float to each channel, and nothing more. And each and every driver will have to do the same.
If a driver would not make use of it at all, it could set it to np.nan (still a float), and ignore it.

Note
Despite the comment being marked as outdated, the file is there, with the exact same path. It's content didn't change by a single byte since this comment.

I also think that instrument specific configs are useful at this stage, maybe even more than instrument specific channels. There may be non-power related parameters specific to some instrument, for example

qibolab/src/qibolab/instruments/qm/components/configs.py

Line 20 in 443dac8

filter: dict[str, float] = field(default_factory=dict)

that we would like to expose in the configs and doing so is simpler if we have instrument specific configs.

Moreover, it may indeed be possible to find a way to unify power related parameters, however this is not trivial and I would certainly prioritize other things over it. Some parameters like attenuation and gain even have opposite effect, so even if it is just for documentation reasons, I would prefer to have them separate and well documented in each driver, than a global thing for which the translation is not very clear.

The only disadvantange I can think of is that we won't be able to write instrument agnostic calibration routines that tune instrument specific parameters (obviously), however the work around is to stick to default channel configs as much as possible when writing drivers and only introduce new configs if it is absolutely necessary. At least this is the approach I will follow for #933.

I also think that instrument specific configs are useful at this stage, maybe even more than instrument specific channels

Instrument-specific is definitely fine, but concerning (instrument, channel)-specific I do not agree.
The elements that are used in one case could be used even in a different one. All instruments are essentially performing the same tasks, and that's why Qibolab makes sense in the first place. So, since the goal of Qibolab is to make an instrument agnostic interface, we should struggle to do that (despite the internals of each instrument are different, and the specific ways they work could be slightly different as well).

Moreover, it may indeed be possible to find a way to unify power related parameters, however this is not trivial and I would certainly prioritize other things over it. Some parameters like attenuation and gain even have opposite effect, so even if it is just for documentation reasons, I would prefer to have them separate and well documented in each driver, than a global thing for which the translation is not very clear.

Here I quite disagree, e.g. since it would be possible for Qibocal to make use of the same parameter even if it had the opposite meaning.
To make an actual example, if you create a routine that plots something as a function of attenuation/gain it would work in both cases, just with a reflected plot. But only if Qibolab exposes a single key, otherwise you'll need device-specific routines.

Moreover, if you have some capability that is specific to some instrument, and only to that, it's still better to expose it as a general one, and set it to None for all the others. At least, it will never be device-specific, and you know what you could do just checking the presence of some value (i.e. testing is not None), instead of querying the specific instrument identity.

alecandido · 2024-06-23T08:00:41Z

src/qibolab/pulses/sequence.py



-class PulseSequence(list):
-    """A collection of scheduled pulses.
+class PulseSequence(defaultdict):


I'd be in favor of a renaming PulseSequence -> Sequence (since there is no other meaningful sequence for the Qibolab user, and internally will be scoped in pulses anyhow). But it's definitely not required.

Would you agree with that? @stavros11 @hay-k

Yes, that should be done. I can do another followup PR just for the rename, since it is going to cause a lot changes.

Then open an issue, assign to the 0.2 milestone, and link it here.

src/qibolab/qubits.py

src/qibolab/platform.py

alecandido · 2024-06-24T14:26:26Z

src/qibolab/platform.py

+            updates: list of updates, where each entry is a dict mapping component name to new config. Later entries
+                     in the list override earlier entries (if they happen to update the same thing).
+        """
+        components = self.components.copy()


Ok, this function is not mutating, but copying.

However, to implement the function that is mutating, you will call this one. And that will mutate and copy.

I'd suggest having an internal function that is mutating without copying, and a wrapper that is avoiding the mutation and copying (e.g. copying self, or just copying components, applying the mutation, and swapping the result with the copy of the original).

I am not sure at what stage of the draft was this commented, but currently the component configs dict in the platform is never updated. The intention is

updates will happen only during execution, and platform always stays at defaults.

if users want to dump updated configs, there is a function that does this by accepting the same updates again. This is supposed to replace the current use case of qibocal, where they mutate the platform itself and then dump it. And now I remember that I have forgotten to implement this point :D

I am not sure at what stage of the draft was this commented,

Again, nothing changed since then, the line is still there:

qibolab/src/qibolab/platform/platform.py

Line 190 in 982c843

components = self.component_configs.copy()

updates will happen only during execution, and platform always stays at defaults.

I don't care much about the platform being the same or mutating. There is almost always a single platform around for each process, it is almost a singleton. And it's updated in very predictable steps, even in Qibocal.

However, even fine if you do not want to update the platform. But I want at least to be able to update a components' dictionary. In-place, without copying.
Then, if I want to copy I will, but that's an extra operation.
If I have 1M components, and I want to update one, here I'm forced to copy 1M, though is definitely not necessary for what is done by the platform.

So, if you do not want to mutate the platform, split the updating function as:

def configs_updates(components, updates): ... # no copy return updated_components class Platform: ... # if you really still need this... def _apply_config_updates(self, updates): components = self.components.copy() return configs_updates(components, updates)

Sure, I will keep an eye on this when we have 1M components.

alecandido · 2024-06-24T14:30:49Z

src/qibolab/platform.py

+        """Apply given list of config updates to the default configuration and
+        return the updated config dict.
+
+        Args:
+            updates: list of updates, where each entry is a dict mapping component name to new config. Later entries
+                     in the list override earlier entries (if they happen to update the same thing).
+        """


I'm trying to simplify the docstrings, and apply some style guidelines (e.g. keep the first line on a single line).

Suggested change

"""Apply given list of config updates to the default configuration and

return the updated config dict.

Args:

updates: list of updates, where each entry is a dict mapping component name to new config. Later entries

in the list override earlier entries (if they happen to update the same thing).

"""

"""Apply updates to the default configuration and return the updated one.

`updates` is a list of updates, where each entry is a dict mapping component

name to new config. Later entries in the list override earlier entries (if

they happen to update the same thing).

"""

;This is the same style of Python docs, and for internal functions is perfect - for the public API we could use the NumPy convention or not, but I'd decide when we'll clarify what the public API will consist of.

alecandido · 2024-06-26T17:32:44Z

@hay-k, whenever you can, please rebase on current 0.2

alecandido · 2024-07-03T08:10:53Z

src/qibolab/instruments/zhinst/constants.py

+"""Shared constants."""
+
+SAMPLING_RATE = 2
+NANO_TO_SECONDS = 1e-9


Couldn't you restrict the conversion to happen in a single place?

not for now. This in fact is not a new file, it is the old utils.py that I renamed, because all other things got removed from it. Not sure why git didn't realize this. So we are indeed getting closer to getting rid of this file :D

So we are indeed getting closer to getting rid of this file :D

Great :)

This in fact is not a new file, it is the old utils.py that I renamed, because all other things got removed from it. Not sure why git didn't realize this.

The file changed too much in a single commit, including the name. And Git was not able to trace it was the same file.
But there is still the overall diff. I realized, no worries ^^

Just an example for NANO_TO_SECONDS [*].

The reason why it is used in places other than executor.py is to convert pulses and sweepers values as well.
However, all these functions are called in executor.py and only there, so there is a single interface where pulses and sweepers are received (and in general exchanged), i.e. Zurich.play(). Couldn't you convert at that level?

This is definitely not required for this PR. But if you're not going to do it here, just open an issue for that.
The goal is not much getting rid of a file, but isolate more of this information, in order to have more modular components (disentangling the web of connections, and reducing possible conversion's mistakes).

[*]: which is the ubiquitous one, while SAMPLING_RATE is used only once outside executor.py

scarrazza · 2024-07-12T08:37:13Z

@hay-k could you please put all your effort in this PR, in particular by fixing tests, before moving to other tasks such as driver's refactoring? Thanks.

Something changed remotely, at the point that even old runs, already passing, are failing once rerun Compare the two attempts in https://github.com/qiboteam/qibolab/actions/runs/10177610824

rust-lang/rustup#3409 (comment)

GitHub is deprecating those based on an old Node version

alecandido · 2024-08-05T14:07:32Z

@hay-k since we now need to move forward, I'd like to merge the PR as is in https://github.com/qiboteam/qibolab/tree/0.2, assuming it is completed on your side.

Let me know if you have anything against, and feel free to merge on your own.
In any case, I will merge it tomorrow, if not happened before.

alecandido reviewed Apr 26, 2024

View reviewed changes

tests/dummy_qrc/qblox/platform.py Outdated Show resolved Hide resolved

alecandido reviewed Apr 26, 2024

View reviewed changes

alecandido added this to the Qibolab 0.2.0 milestone Apr 26, 2024

stavros11 reviewed Apr 26, 2024

View reviewed changes

src/qibolab/dummy/platform.py Outdated Show resolved Hide resolved

stavros11 reviewed Apr 26, 2024

View reviewed changes

src/qibolab/qubits.py Show resolved Hide resolved

src/qibolab/pulses/sequence.py Outdated Show resolved Hide resolved

src/qibolab/channel.py Outdated Show resolved Hide resolved

alecandido reviewed Apr 30, 2024

View reviewed changes

alecandido mentioned this pull request May 2, 2024

Remove couplers #891

Merged

alecandido force-pushed the channel-api branch from 7660a67 to 92cdddd Compare May 3, 2024 14:15

alecandido mentioned this pull request May 3, 2024

Serialization blockers #882

Closed

14 tasks

alecandido reviewed May 6, 2024

View reviewed changes

src/qibolab/channel/__init__.py Outdated Show resolved Hide resolved

alecandido reviewed May 6, 2024

View reviewed changes

src/qibolab/serialize.py Show resolved Hide resolved

hay-k mentioned this pull request May 6, 2024

Upgrade platform with ZI instruments to the new channel API qiboteam/qibolab_platforms_qrc#134

Closed

alecandido reviewed May 14, 2024

View reviewed changes

hay-k mentioned this pull request May 16, 2024

Collect frequencies before processing sweeps #902

Merged

4 tasks

hay-k force-pushed the channel-api branch from b6082fc to 3ecfba7 Compare May 24, 2024 08:19

hay-k mentioned this pull request Jun 21, 2024

Expose mixer calibration parameters in Qblox drivers #900

Open

alecandido reviewed Jun 23, 2024

View reviewed changes

alecandido reviewed Jun 24, 2024

View reviewed changes

alecandido force-pushed the 0.2 branch from f829e8f to 33b733d Compare June 25, 2024 16:34

hay-k force-pushed the channel-api branch 4 times, most recently from 822d3b0 to 66a0609 Compare July 2, 2024 13:02

alecandido reviewed Jul 3, 2024

View reviewed changes

hay-k and others added 17 commits August 5, 2024 14:34

temporarily disable non 0.2 compatible platforms

73e5887

update unit test platform with ZI instruments

9944e95

change confusing name of platform

a67d222

recover function lost during rebase

11f672b

improve type annotation

5f1401c

implement init from dict

3b6b5f3

update documentation

8dceab7

some port tests

b406f31

rename component_configs to configs

2189a90

rename the sweep method to play for zi executor

3d24096

Some port tests

bee7498

remove un-necessary method load_component_config

bfd7cc4

revert accidental change in pyproject.toml

5006b74

rename measure to probe

e237aee

dump platform with updates

5005f93

update documentation

84dfaec

chore: Fix rebase leftover

efefe5d

alecandido force-pushed the channel-api branch from 9e2fcd2 to efefe5d Compare August 5, 2024 13:16

alecandido added 4 commits August 5, 2024 15:20

ci: Temporarily block automated run of Rust checks

a14ff78

Something changed remotely, at the point that even old runs, already passing, are failing once rerun Compare the two attempts in https://github.com/qiboteam/qibolab/actions/runs/10177610824

ci: Replace rustup update with external installation action

3572165

rust-lang/rustup#3409 (comment)

ci: Update actions in local workflows

4522fa7

GitHub is deprecating those based on an old Node version

fix: Fix further rebase leftover

c1747ad

alecandido mentioned this pull request Aug 5, 2024

Initial implementation for Keysight QCS #944

Open

8 tasks

ci: Attempt switching back to macos latest

0eaf775

alecandido mentioned this pull request Aug 5, 2024

Replace results with bare arrays #940

Merged

2 tasks

hay-k merged commit ce8505d into 0.2 Aug 6, 2024
21 of 22 checks passed

alecandido deleted the channel-api branch August 6, 2024 06:42

This was referenced Aug 28, 2024

Unrolling and parallel sweeps for QM #1008

Merged

Remove outdated drivers #1013

Merged

	def append(self, other: "PulseSequence") -> None:
	def extend(self, other: "PulseSequence") -> None:

	if delay := round(max_duration - duration, int(1 / tol)) > 0:
	delay = round(max_duration - duration, int(1 / tol))
	if delay > 0:

		"""Component (a.k.a. logical component) is a concept that is part of the
		qibolab interface exposed to its users.

	"""Component (a.k.a. logical component) is a concept that is part of the
	qibolab interface exposed to its users.
	"""A component of the user interface (a.k.a. logical component).

Channel api #885

Channel api #885

Conversation

hay-k commented Apr 26, 2024 • edited Loading

alecandido left a comment

Choose a reason for hiding this comment

stavros11 left a comment

Choose a reason for hiding this comment

hay-k commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido commented May 3, 2024

stavros11 commented May 4, 2024

alecandido commented May 4, 2024

alecandido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido commented Jun 26, 2024

Choose a reason for hiding this comment

hay-k Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scarrazza commented Jul 12, 2024

alecandido commented Aug 5, 2024

hay-k commented Apr 26, 2024 •

edited

Loading

hay-k Jul 17, 2024 •

edited

Loading