qiboteam / qibolab

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

Drop Pulse subclasses #767

Closed alecandido closed 7 months ago

alecandido commented 7 months ago

Use the .type attribute instead, which was already present (and they were quite a duplicate of each other).

Moreover, the .copy() and .shallow_copy() method of the pulse class have been removed, since they were mostly required to deal with the subclasses themselves. As a replacement, the copy.copy() and copy.deepcopy() standard library functions are suggested.

alecandido commented 7 months ago

@stavros11 @PiergiorgioButtarini @hay-k @Jacfomg while replacing the subclasses by using Pulse.type attribute, I realized that even that might be superfluous.

Essentially, the type of the pulse is telling you where this object will be played on. I have two similar objections to this procedure:

So, as a first proposal, we could try to identify the type using the channel. Later on, we could try to kick the channel (and qubit) out of the pulse (as @hay-k was also suggesting at some point, or something close to) and move this information somewhere else (potentially the sequence).

The second step is less appealing, because it's just moving the complexity from one place to the other. Still, it could help to better isolate the components.

codecov[bot] commented 7 months ago

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (dea5405) 61.84% compared to head (f10ba3a) 61.72%.

Files Patch % Lines
src/qibolab/instruments/qblox/sequencer.py 33.33% 2 Missing :warning:
src/qibolab/pulses.py 75.00% 2 Missing :warning:
src/qibolab/instruments/qblox/cluster_qcm_bb.py 50.00% 1 Missing :warning:
src/qibolab/instruments/qblox/cluster_qcm_rf.py 50.00% 1 Missing :warning:
src/qibolab/instruments/qblox/cluster_qrm_rf.py 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## simplify-pulse-2 #767 +/- ## ==================================================== - Coverage 61.84% 61.72% -0.13% ==================================================== Files 47 47 Lines 5748 5722 -26 ==================================================== - Hits 3555 3532 -23 + Misses 2193 2190 -3 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/767/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/767/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `61.72% <82.05%> (-0.13%)` | :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.

stavros11 commented 7 months ago
  • there is already such an attribute, it is the Pulse.channel, so we could well retrieve the information from there
  • it doesn't really make the difference whether a rectangular pulse is a drive or readout pulse, they are both rectangular, and they can have similar parameters

I believe both these points are right, it is redundant to have both type and channel. However, the main issue right now is that not all drivers are using pulse.channel. Some are looking at pulse.qubit to know where to play the pulse (mostly my fault) and in this case pulse.type is needed.

In any case I would proceed PR since it is at least removing a third redundancy (the subclassing) and for sure come back to the channel issue later.

alecandido commented 7 months ago

In any case I would proceed PR since it is at least removing a third redundancy (the subclassing) and for sure come back to the channel issue later.

Agreed. I raised the question because it became relevant. My current goals are to seize the low-hanging fruits first, and to greatly reduce the complexity of pulses.py with things that mostly regard only pulses. As soon as that will be close to completion (hopefully next week) I would try to start examining the interactions between pulses and everything else (qubit and channels are relevant, but most likely native are easier, so I'd start there).

Jacfomg commented 7 months ago

@stavros11 @PiergiorgioButtarini @hay-k @Jacfomg while replacing the subclasses by using Pulse.type attribute, I realized that even that might be superfluous.

Essentially, the type of the pulse is telling you where this object will be played on. I have two similar objections to this procedure:

* there is already such an attribute, it is the `Pulse.channel`, so we could well retrieve the information from there

* it doesn't really make the difference whether a rectangular pulse is a drive or readout pulse, they are both rectangular, and they can have similar parameters

So, as a first proposal, we could try to identify the type using the channel. Later on, we could try to kick the channel (and qubit) out of the pulse (as @hay-k was also suggesting at some point, or something close to) and move this information somewhere else (potentially the sequence).

The second step is less appealing, because it's just moving the complexity from one place to the other. Still, it could help to better isolate the components.

It makes sense since we have the Pulse.channel, but when I wrote the Zurich driver I made use a lot of the Pulse.type. Btw, since we plan on changing the Pulse maybe we could get rid of the example notebook.