qiboteam / qibolab

Quantum hardware module and drivers for Qibo.
https://qibo.science
Apache License 2.0
43 stars 16 forks source link

Qblox #1088

Open alecandido opened 4 weeks ago

alecandido commented 4 weeks ago

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

codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 5.11364% with 167 lines in your changes missing coverage. Please review.

Project coverage is 51.69%. Comparing base (f7a8883) to head (4a14942).

Files with missing lines Patch % Lines
src/qibolab/_core/instruments/qblox/cluster.py 0.00% 96 Missing :warning:
src/qibolab/_core/instruments/qblox/platform.py 0.00% 43 Missing :warning:
src/qibolab/_core/instruments/qblox/sequence.py 0.00% 24 Missing :warning:
src/qibolab/instruments/qblox.py 0.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## parse-q1asm #1088 +/- ## =============================================== - Coverage 54.35% 51.69% -2.67% =============================================== Files 66 70 +4 Lines 3170 3341 +171 =============================================== + Hits 1723 1727 +4 - Misses 1447 1614 +167 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/1088/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibolab/pull/1088/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `51.69% <5.11%> (-2.67%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alecandido commented 3 weeks ago

@stavros11 I start wondering why do we have this distinction at all: https://github.com/qiboteam/qibolab/blob/f74c334d278f9c45de84f38e0f719a25c0dc1fc5/src/qibolab/_core/components/channels.py#L32-L35

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 commented 3 weeks ago

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 commented 3 weeks ago

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

alecandido commented 1 week ago

@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 commented 4 days ago

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