Migrate Pulse to dataclass

[alecandido]

First batch of #683.

Since these updates are going to affect many places, the idea is to attempt bundling changes in a few units, and make sure everything is still working every time (also to avoid a single huge PR).

Checklist:

• Reviewers confirm new code works as expected.
• Tests are passing.
• Coverage does not decrease.
• Documentation is updated.

[codecov]

Codecov Report

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

Comparison is base (da6d1b9) 63.58% compared to head (e6168d4) 62.94%.

Files	Patch %	Lines
src/qibolab/pulses.py	89.79%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #709      +/-   ##
==========================================
- Coverage   63.58%   62.94%   -0.65%     
==========================================
  Files          49       49              
  Lines        6539     6399     -140     
==========================================
- Hits         4158     4028     -130     
+ Misses       2381     2371      -10     

Flag	Coverage Δ
unittests	62.94% <89.79%> (-0.65%)	⬇️

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]

@stavros11 @andrea-pasquale at this point I would actually also like to drop ._if: it is only used by Qblox

❯ rg '\._if'
tests/test_pulses.py
694:        2 * np.pi * pulse._if * pulse.start / 1e9
697:        i, q, num_samples, pulse._if, global_phase + pulse.relative_phase, sampling_rate
719:        == f"Modulated_Waveform_I(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
723:        == f"Modulated_Waveform_Q(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
764:        i, q, num_samples, pulse._if, global_phase + pulse.relative_phase, sampling_rate
786:        == f"Modulated_Waveform_I(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
790:        == f"Modulated_Waveform_Q(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
834:        2 * np.pi * pulse._if * pulse.start / 1e9
837:        i, q, num_samples, pulse._if, global_phase + pulse.relative_phase, sampling_rate
859:        == f"Modulated_Waveform_I(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
863:        == f"Modulated_Waveform_Q(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"

src/qibolab/instruments/qblox/controller.py
183:                    pulse._if = int(pulse.frequency - pulse_channel.lo_frequency)

src/qibolab/pulses.py
168:        if abs(pulse._if) * 2 > sampling_rate:
176:            2 * np.pi * pulse._if * time + global_phase + pulse.relative_phase
179:            2 * np.pi * pulse._if * time + global_phase + pulse.relative_phase
200:        modulated_waveform_i.serial = f"Modulated_Waveform_I(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
202:        modulated_waveform_q.serial = f"Modulated_Waveform_Q(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"

@PiergiorgioButtarini is it possible to store this information anywhere else in Qblox?

[andrea-pasquale]

@stavros11 @andrea-pasquale at this point I would actually also like to drop ._if: it is only used by Qblox

❯ rg '\._if'
tests/test_pulses.py
694:        2 * np.pi * pulse._if * pulse.start / 1e9
697:        i, q, num_samples, pulse._if, global_phase + pulse.relative_phase, sampling_rate
719:        == f"Modulated_Waveform_I(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
723:        == f"Modulated_Waveform_Q(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
764:        i, q, num_samples, pulse._if, global_phase + pulse.relative_phase, sampling_rate
786:        == f"Modulated_Waveform_I(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
790:        == f"Modulated_Waveform_Q(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
834:        2 * np.pi * pulse._if * pulse.start / 1e9
837:        i, q, num_samples, pulse._if, global_phase + pulse.relative_phase, sampling_rate
859:        == f"Modulated_Waveform_I(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
863:        == f"Modulated_Waveform_Q(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"

src/qibolab/instruments/qblox/controller.py
183:                    pulse._if = int(pulse.frequency - pulse_channel.lo_frequency)

src/qibolab/pulses.py
168:        if abs(pulse._if) * 2 > sampling_rate:
176:            2 * np.pi * pulse._if * time + global_phase + pulse.relative_phase
179:            2 * np.pi * pulse._if * time + global_phase + pulse.relative_phase
200:        modulated_waveform_i.serial = f"Modulated_Waveform_I(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"
202:        modulated_waveform_q.serial = f"Modulated_Waveform_Q(num_samples = {num_samples}, amplitude = {format(pulse.amplitude, '.6f').rstrip('0').rstrip('.')}, shape = {str(pulse.shape)}, frequency = {format(pulse._if, '_')}, phase = {format(global_phase + pulse.relative_phase, '.6f').rstrip('0').rstrip('.')})"

@PiergiorgioButtarini is it possible to store this information anywhere else in Qblox?

If it possible I would also drop it at this point. I believe that in theory the if_ is completely determined by the frequency of the pulse that we want to send and the lo frequency. I don't know exactly how the qblox driver works but if during the pulse execution it could access the frequency of the los I think it should be relatively easy to remove the if_ completely.

[alecandido]

I don't know exactly how the qblox driver works but if during the pulse execution it could access the frequency of the los I think it should be relatively easy to remove the if_ completely.

This I'm trying to understand myself. But, as you can see from the quoted line, Qblox is actually setting this information from the LO at some point, a point when the pulse is accessible (thus during an invocation of some kind of play).
So, it is only used to save the information in the meanwhile. We just need to postpone the computation, and propagate the LO instead of the IF.

[alecandido]

Tested on Qblox, working ✅

Pytest report

alessandro.candido@saadiyat:~/qibolab$ srun -p qw5q_gold bash -c 'export QIBOLAB_PLATFORMS=$HOME/qibolab_platforms_qrc; poetry run pytest -m qpu --platform qw5q_gold'
============================= test session starts ==============================
platform linux -- Python 3.10.12, pytest-7.4.3, pluggy-1.3.0
rootdir: /nfs/users/alessandro.candido/qibolab
configfile: pyproject.toml
testpaths: tests/
plugins: mock-3.12.0, env-1.1.3, dash-2.14.2, cov-4.1.0
collected 2294 items / 2239 deselected / 55 selected

tests/test_backends.py xxx                                               [  5%]
tests/test_instruments_erasynth.py sss                                   [ 10%]
tests/test_instruments_qblox_cluster_qcm_bb.py ....                      [ 18%]
tests/test_instruments_qblox_cluster_qcm_rf.py ....                      [ 25%]
tests/test_instruments_qblox_cluster_qrm_rf.py ....                      [ 32%]
tests/test_instruments_qutech.py sss                                     [ 38%]
tests/test_instruments_rfsoc.py sssss                                    [ 47%]
tests/test_instruments_rohde_schwarz.py ....                             [ 54%]
tests/test_instruments_zhinst.py sss                                     [ 60%]
tests/test_platform.py ...s.......sss                                    [ 85%]
tests/test_result_shapes.py ........                                     [100%]

[...]

= 34 passed, 18 skipped, 2239 deselected, 3 xfailed, 14 warnings in 115.71s (0:01:55) =

[alecandido]

Tested on Zurich, broken ❌

Pytest report

============================= test session starts ==============================
platform linux -- Python 3.10.12, pytest-7.4.3, pluggy-1.3.0
rootdir: /nfs/users/alessandro.candido/qibolab
configfile: pyproject.toml
testpaths: tests/
plugins: mock-3.12.0, env-1.1.3, dash-2.14.2, cov-4.1.0
collected 2294 items / 2239 deselected / 55 selected

tests/test_backends.py xxx                                               [  5%]
tests/test_instruments_erasynth.py sss                                   [ 10%]
tests/test_instruments_qblox_cluster_qcm_bb.py ssss                      [ 18%]
tests/test_instruments_qblox_cluster_qcm_rf.py ssss                      [ 25%]
tests/test_instruments_qblox_cluster_qrm_rf.py ssss                      [ 32%]
tests/test_instruments_qutech.py sss                                     [ 38%]
tests/test_instruments_rfsoc.py sssss                                    [ 47%]
tests/test_instruments_rohde_schwarz.py ....                             [ 54%]
tests/test_instruments_zhinst.py FF.                                     [ 60%]
tests/test_platform.py ...........sss                                    [ 85%]
tests/test_result_shapes.py ........                                     [100%]

=================================== FAILURES ===================================
____________________ test_experiment_execute_pulse_sequence ____________________

connected_platform = Platform(name='iqm5q', qubits={0: Qubit(name=0, bare_resonator_frequency=5225320060, readout_frequency=5227920060, dri...ed=True, two_qubit_native_types=<NativeGates.CZ: 64>, topology=<networkx.classes.graph.Graph object at 0x7ff081d47160>)
instrument = <qibolab.instruments.zhinst.Zurich object at 0x7ff082763730>

    @pytest.mark.qpu
    def test_experiment_execute_pulse_sequence(connected_platform, instrument):
        platform = connected_platform
        platform.setup()
    
        sequence = PulseSequence()
>       qubits = {0: platform.qubits[0], "c0": platform.qubits["c0"]}
E       KeyError: 'c0'

tests/test_instruments_zhinst.py:769: KeyError
---------------------------- Captured stdout setup -----------------------------
Connected to: Rohde&Schwarz SGS100A (serial:1416.0505k02/113302, firmware:4.2.76.0-4.30.046.295) in 0.20s
------------------------------ Captured log setup ------------------------------
WARNING  laboneq.controller.devices.device_zi:device_zi.py:246 HDAWG:dev8660: Include the device options 'HDAWG8/' in the device setup ('options' field of the 'instruments' list in the device setup descriptor). This will become a strict requirement in the future.
WARNING  laboneq.controller.devices.device_zi:device_zi.py:246 HDAWG:dev8673: Include the device options 'HDAWG4/' in the device setup ('options' field of the 'instruments' list in the device setup descriptor). This will become a strict requirement in the future.
WARNING  laboneq.controller.devices.device_zi:device_zi.py:246 SHFQC/QA:dev12146: Include the device options 'SHFQC/QC6CH' in the device setup ('options' field of the 'instruments' list in the device setup descriptor). This will become a strict requirement in the future.
______________________ test_experiment_sweep_2d_specific _______________________

connected_platform = Platform(name='iqm5q', qubits={0: Qubit(name=0, bare_resonator_frequency=5225320060, readout_frequency=5227920060, dri...ed=True, two_qubit_native_types=<NativeGates.CZ: 64>, topology=<networkx.classes.graph.Graph object at 0x7ff081d47160>)
instrument = <qibolab.instruments.zhinst.Zurich object at 0x7ff082763730>

    @pytest.mark.qpu
    def test_experiment_sweep_2d_specific(connected_platform, instrument):
        platform = connected_platform
        platform.setup()
    
        sequence = PulseSequence()
        qubits = {0: platform.qubits[0]}
    
        swept_points = 5
        sequence = PulseSequence()
        ro_pulses = {}
        qd_pulses = {}
        for qubit in qubits:
            qd_pulses[qubit] = platform.create_RX_pulse(qubit, start=0)
            sequence.add(qd_pulses[qubit])
            ro_pulses[qubit] = platform.create_qubit_readout_pulse(
                qubit, start=qd_pulses[qubit].finish
            )
            sequence.add(ro_pulses[qubit])
    
        parameter1 = Parameter.relative_phase
        parameter2 = Parameter.frequency
    
        parameter_range_1 = (
            np.random.rand(swept_points)
            if parameter1 is Parameter.amplitude
            else np.random.randint(swept_points, size=swept_points)
        )
    
        parameter_range_2 = (
            np.random.rand(swept_points)
            if parameter2 is Parameter.amplitude
            else np.random.randint(swept_points, size=swept_points)
        )
    
        sweepers = []
        sweepers.append(Sweeper(parameter1, parameter_range_1, pulses=[qd_pulses[qubit]]))
        sweepers.append(Sweeper(parameter2, parameter_range_2, pulses=[qd_pulses[qubit]]))
    
        options = ExecutionParameters(
            relaxation_time=300e-6,
            acquisition_type=AcquisitionType.INTEGRATION,
            averaging_mode=AveragingMode.CYCLIC,
        )
    
        results = platform.sweep(
            sequence,
            options,
            sweepers[0],
            sweepers[1],
        )
    
>       assert len(results[ro_pulses[qubit].serial]) > 0
E       TypeError: object of type 'AveragedIntegratedResults' has no len()

tests/test_instruments_zhinst.py:856: TypeError

[...]

FAILED tests/test_instruments_zhinst.py::test_experiment_execute_pulse_sequence
FAILED tests/test_instruments_zhinst.py::test_experiment_sweep_2d_specific - ...
= 2 failed, 24 passed, 26 skipped, 2239 deselected, 3 xfailed, 14 warnings in 172.07s (0:02:52) =

But it's not my fault, it's broken exactly in the same way on main.

[alecandido]

Unfortunately, qiboteam/qibolab_platforms_qrc#88 is outdated, so I don't have a runcard to test tii1q_b1, and thus I can not test whether I'm breaking the RFSoC or not...

[alecandido]

Btw, I obviously can't test on QM, unless someone can point me out a suitable platform to do it...

With this, I'm ready for the review :)

[stavros11]

Thanks @alecandido. In terms of hardware deployment, I also played with this branch a bit and it seems to work so it should be safe to merge. Regarding the Zurich tests, I wouldn't worry on this PR, but we should certainly fix them in a different one.

For QM, I would also say it's not worth spending time testing now as I will be doing some major updates in the driver soon to support the Octaves. Also the qua version in our pyproject is very old - they finally put back support for py311 so I will update it when I do the driver. If you really want to try I can provide a runcard (dummy since it is not connected to anything, but still using the real instrument), but not really required.

Other than that, I understand that this PR is only a first step and there are several TODOs remaining so it should be fine to even merge as it is. One thing that could still be addressed here is the Pulse subclasses (ReadoutPulse, DrivePulse, etc.), I think you did not touch them, but in principle they could also be dataclasses. Unless you have a different plan for them, such as dropping them at some point (since they're kind of duplicating the pulse.type attribute).

Two other things that we should do in a different PR (to avoid large changes) but in principle we could do immediately after are:

1. Create a pulse module (directory) and seperate pulse, shape and sequence to different files,
2. Move plot methods outside the objects (potentially in another file under the pulse module).

These are trivial since it is just copy-pasting existing code but I think it would simplify the review of later changes on these files. Maybe (1) won't be as useful if we manage to simplify pulses a lot, but I think some structure would still help.

[stavros11]

This I'm trying to understand myself. But, as you can see from the quoted line, Qblox is actually setting this information from the LO at some point, a point when the pulse is accessible (thus during an invocation of some kind of play). So, it is only used to save the information in the meanwhile. We just need to postpone the computation, and propagate the LO instead of the IF.

It is certainly possible to do this and drop _if from the Pulse, but looking at the qblox thing, I think some small refactoring in the driver is needed to make it work. So I would even merge this PR and open an issue to address this from the qblox side. Alternatively you could even drop it from the pulse now and let qblox "inject" it as a new attribute when needed. I believe Python allows it, it's not the best practice, but at least we make sure that is only qblox that is using it.

[alecandido]

Unless you have a different plan for them, such as dropping them at some point (since they're kind of duplicating the pulse.type attribute).

Indeed, the plan is actually to drop. I forgot to spell it out explicitly in #683, where I only detailed the very few additions to main Pulse classes (that is the rationale to drop them as duplicated of the .type attribute, but this remained implicit).

Create a pulse module (directory) and seperate pulse, shape and sequence to different files,

Agreed. I'm trimming the file, but even trimmed it will keep doing quite a bunch of things, so it is useful to split as well.

Move plot methods outside the objects (potentially in another file under the pulse module).

Indeed. I was thinking about a plot module top-level, but plotting is something specific to pulses and their sequences, so it's even nicer as a module in a pulse subpackage.

[alecandido]

Thanks @alecandido. In terms of hardware deployment, I also played with this branch a bit and it seems to work so it should be safe to merge. Regarding the Zurich tests, I wouldn't worry on this PR, but we should certainly fix them in a different one.

For QM, I would also say it's not worth spending time testing now as I will be doing some major updates in the driver soon to support the Octaves. Also the qua version in our pyproject is very old - they finally put back support for py311 so I will update it when I do the driver. If you really want to try I can provide a runcard (dummy since it is not connected to anything, but still using the real instrument), but not really required.

About hw deployment I wanted to be sure that I was not breaking currently working and used drivers.
About the other ones, it's difficult to promise anything, and I hope that after the simplifications it will be simpler to update (in case I'll break anything).

Other than that, I understand that this PR is only a first step and there are several TODOs remaining so it should be fine to even merge as it is.

Indeed, I already opened simplify-pulse-2 locally. But I wanted to avoid a single huge PR, even because they are all potentially breaking, and I wanted to deal with the issues one by one.