Repository: qiboteam/qibolab

Issue:
Define qubit results
              Aren't you overwriting `results[acquisition.qubit]`?

`acquisition` is always the same for all `result` (outer loop), and then `acquisition.qubit` as well. Isn't it?

_Originally posted by @alecandido in https://github.com/qiboteam/qibolab/pull/738#discussion_r1494463241_
            
Currently `platform.execute_pulse_sequence` (and related execution methods) returns a dictionary that contains pulse serials and qubit names. If the sequence contains more than one readout per qubit, it is not well defined whether the qubit entries should contain all acquired measurements or only the ones from the latest readout pulse.

Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/qiboteam/qibolab/pull/964?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 43.94%. Comparing base [(`4abda92`)](https://app.codecov.io/gh/qiboteam/qibolab/commit/4abda924dcb5b27ca1d67c71efa0f5197852513d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) to head [(`adee9c4`)](https://app.codecov.io/gh/qiboteam/qibolab/commit/adee9c4b537d0ffecbc0bc5b53ed3f7afe2841e3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam).

<details><summary>Additional details and impacted files</summary>


```diff
@@                 Coverage Diff                 @@
##           drop-pulse-type     #964      +/-   ##
===================================================
+ Coverage            43.89%   43.94%   +0.05%     
===================================================
  Files                   81       81              
  Lines                 5586     5591       +5     
===================================================
+ Hits                  2452     2457       +5     
  Misses                3134     3134              
```

| [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/964/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/964/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `43.94% <100.00%> (+0.05%)` | :arrow_up: |

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.

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/qiboteam/qibolab/pull/964?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam).

alecandido: Most of the work was of course done in #940, but here I'd like to discuss another possible breaking change, i.e.
https://github.com/qiboteam/qibolab/blob/adee9c4b537d0ffecbc0bc5b53ed3f7afe2841e3/src/qibolab/platform/platform.py#L317-L321
we are now creating lists for each pulse, but in principle pulses may be unique, and we could just keep the results flatter (replacing the `dict[PulseId, list[Result]]` with `dict[PulseId, Result]`).

Of course, this is only relevant for unrolling, since in all the other cases there is a single pulse with the given id, and if there are many results associated (e.g. as in sweepers) the structure is internal to the `Results` array.

The drawback of the proposed scenario is that we lift to the user the burden of generating unique acquisition pulses (of course, we can validate the sequence to check for duplicates) making copies while generating the sequences (instead of reusing the same pulse - but a `.copy()` or `copy.copy()` call is sufficient), while those of the current situation are that you're almost always receiving lists of one element, and that you need to explicitly avoid copies, if you want to keep the results together.
stavros11: > The drawback of the proposed scenario is that we lift to the user the burden of generating unique acquisition pulses (of course, we can validate the sequence to check for duplicates) making copies while generating the sequences (instead of reusing the same pulse - but a `.copy()` or `copy.copy()` call is sufficient), while those of the current situation are that you're almost always receiving lists of one element, and that you need to explicitly avoid copies, if you want to keep the results together.

I would say this solution is acceptable. We just need to validate in `platform.execute` that every pulse appears at most once in the given list of sequences (not even in different sequences that are given in the same list). I don't see any serious issue with that limitation, but if there is we could even relax it by checking only acquisition pulses (and allow reusing drive or other types), as only acquisition is relevant for results.
alecandido: > I don't see any serious issue with that limitation, but if there is we could even relax it by checking only acquisition pulses (and allow reusing drive or other types), as only acquisition is relevant for results.

Then, I will rebase, provide the check (and a couple of tests), and then ask for a review.

Patch:
diff --git a/doc/source/getting-started/experiment.rst b/doc/source/getting-started/experiment.rst
index 16d7685a..221d94bf 100644
--- a/doc/source/getting-started/experiment.rst
+++ b/doc/source/getting-started/experiment.rst
@@ -238,7 +238,7 @@ We leave to the dedicated tutorial a full explanation of the experiment, but her
     _, acq = next(iter(sequence.acquisitions))
 
     # plot the results
-    amplitudes = magnitude(results[acq.id][0])
+    amplitudes = magnitude(results[acq.id])
     frequencies = sweeper.values
 
     plt.title("Resonator Spectroscopy")
diff --git a/doc/source/tutorials/calibration.rst b/doc/source/tutorials/calibration.rst
index ff5620d7..090c3833 100644
--- a/doc/source/tutorials/calibration.rst
+++ b/doc/source/tutorials/calibration.rst
@@ -74,7 +74,7 @@ In few seconds, the experiment will be finished and we can proceed to plot it.
     import matplotlib.pyplot as plt
 
     acq = sequence.acquisitions[0][1]
-    amplitudes = magnitude(results[acq.id][0])
+    amplitudes = magnitude(results[acq.id])
     frequencies = sweeper.values
 
     plt.title("Resonator Spectroscopy")
@@ -165,7 +165,7 @@ We can now proceed to launch on hardware:
     results = platform.execute([sequence], options, [[sweeper]])
 
     _, acq = next(iter(sequence.acquisitions))
-    amplitudes = magnitude(results[acq.id][0])
+    amplitudes = magnitude(results[acq.id])
     frequencies = sweeper.values
 
     plt.title("Resonator Spectroscopy")
@@ -258,12 +258,12 @@ and its impact on qubit states in the IQ plane.
     plt.xlabel("I [a.u.]")
     plt.ylabel("Q [a.u.]")
     plt.scatter(
-        results_one[acq1.id][0],
-        results_one[acq1.id][0],
+        results_one[acq1.id],
+        results_one[acq1.id],
         label="One state",
     )
     plt.scatter(
-        *unpack(results_zero[acq0.id][0]),
+        *unpack(results_zero[acq0.id]),
         label="Zero state",
     )
     plt.show()
diff --git a/src/qibolab/backends.py b/src/qibolab/backends.py
index 4b460849..73552a20 100644
--- a/src/qibolab/backends.py
+++ b/src/qibolab/backends.py
@@ -1,5 +1,3 @@
-from collections import deque
-
 import numpy as np
 from qibo import __version__ as qibo_version
 from qibo.backends import NumpyBackend
@@ -69,8 +67,11 @@ class QibolabBackend(NumpyBackend):
                 containing the readout measurement shots. This is created in ``execute_circuit``.
         """
         for gate, sequence in measurement_map.items():
-            _samples = (readout[acq.id] for _, acq in sequence.acquisitions)
-            samples = list(filter(lambda x: x is not None, _samples))
+            samples = [
+                s
+                for s in (readout[acq.id] for _, acq in sequence.acquisitions)
+                if s is not None
+            ]
             gate.result.backend = self
             gate.result.register_samples(np.array(samples).T)
 
@@ -106,7 +107,7 @@ class QibolabBackend(NumpyBackend):
             [sequence],
             ExecutionParameters(nshots=nshots),
         )
-        readout = {k: v[0] for k, v in readout_.items()}
+        readout = {k: v for k, v in readout_.items()}
 
         self.platform.disconnect()
 
@@ -153,15 +154,12 @@ class QibolabBackend(NumpyBackend):
         self.platform.disconnect()
 
         results = []
-        readout = {k: deque(v) for k, v in readout.items()}
         for circuit, measurement_map in zip(circuits, measurement_maps):
             results.append(
                 MeasurementOutcomes(circuit.measurements, self, nshots=nshots)
             )
             for gate, sequence in measurement_map.items():
-                samples = [
-                    readout[acq.id].popleft() for _, acq in sequence.acquisitions
-                ]
+                samples = [readout[acq.id] for _, acq in sequence.acquisitions]
                 gate.result.backend = self
                 gate.result.register_samples(np.array(samples).T)
         return results
diff --git a/src/qibolab/compilers/compiler.py b/src/qibolab/compilers/compiler.py
index 20b3ddd1..9031901b 100644
--- a/src/qibolab/compilers/compiler.py
+++ b/src/qibolab/compilers/compiler.py
@@ -197,11 +197,13 @@ class Compiler:
         # process circuit gates
         for moment in circuit.queue.moments:
             for gate in {x for x in moment if x is not None}:
-                sequence += self._compile_gate(gate, platform, channel_clock)
+                gate_seq = self._compile_gate(gate, platform, channel_clock)
 
                 # register readout sequences to ``measurement_map`` so that we can
                 # properly map acquisition results to measurement gates
                 if isinstance(gate, gates.M):
-                    measurement_map[gate] = self.get_sequence(gate, platform)
+                    measurement_map[gate] = gate_seq
+
+                sequence += gate_seq
 
         return sequence.trim(), measurement_map
diff --git a/src/qibolab/native.py b/src/qibolab/native.py
index a2bed8ee..2c4ad324 100644
--- a/src/qibolab/native.py
+++ b/src/qibolab/native.py
@@ -1,4 +1,5 @@
 from abc import ABC, abstractmethod
+from copy import deepcopy
 from typing import Annotated, Optional
 
 import numpy as np
@@ -78,7 +79,7 @@ class FixedSequenceFactory(Native):
     """Simple factory for a fixed arbitrary sequence."""
 
     def create_sequence(self) -> PulseSequence:
-        return self.copy()
+        return deepcopy(self)
 
 
 class MissingNative(RuntimeError):
diff --git a/src/qibolab/platform/platform.py b/src/qibolab/platform/platform.py
index f35ac34b..1012d29a 100644
--- a/src/qibolab/platform/platform.py
+++ b/src/qibolab/platform/platform.py
@@ -1,11 +1,10 @@
 """A platform for executing quantum algorithms."""
 
-from collections import defaultdict
 from collections.abc import Iterable
 from dataclasses import dataclass, field
 from math import prod
 from pathlib import Path
-from typing import Any, Literal, Optional, TypeVar, Union
+from typing import Literal, Optional, TypeVar, Union
 
 from qibo.config import log, raise_error
 
@@ -15,7 +14,9 @@ from qibolab.execution_parameters import ExecutionParameters
 from qibolab.identifier import ChannelId, QubitId, QubitPairId
 from qibolab.instruments.abstract import Controller, Instrument, InstrumentId
 from qibolab.parameters import NativeGates, Parameters, Settings, update_configs
+from qibolab.pulses import PulseId
 from qibolab.qubits import Qubit
+from qibolab.result import Result
 from qibolab.sequence import PulseSequence
 from qibolab.sweeper import ParallelSweepers
 from qibolab.unrolling import Bounds, batch
@@ -37,6 +38,11 @@ def default(value: Optional[T], default: T) -> T:
     return value if value is not None else default
 
 
+def _channels_map(elements: QubitMap) -> dict[ChannelId, QubitId]:
+    """Map channel names to element (qubit or coupler)."""
+    return {ch: id for id, el in elements.items() for ch in el.channels}
+
+
 def estimate_duration(
     sequences: list[PulseSequence],
     options: ExecutionParameters,
@@ -54,9 +60,13 @@ def estimate_duration(
     )
 
 
-def _channels_map(elements: QubitMap) -> dict[ChannelId, QubitId]:
-    """Map channel names to element (qubit or coupler)."""
-    return {ch: id for id, el in elements.items() for ch in el.channels}
+def _unique_acquisitions(sequences: list[PulseSequence]) -> bool:
+    """Check unique acquisition identifiers."""
+    ids = []
+    for seq in sequences:
+        ids += (p.id for _, p in seq.acquisitions)
+
+    return len(ids) == len(set(ids))
 
 
 @dataclass
@@ -202,7 +212,7 @@ class Platform:
         options: ExecutionParameters,
         configs: dict[str, Config],
         sweepers: list[ParallelSweepers],
-    ):
+    ) -> dict[PulseId, Result]:
         """Execute sequences on the controllers."""
         result = {}
 
@@ -219,7 +229,7 @@ class Platform:
         sequences: list[PulseSequence],
         options: ExecutionParameters,
         sweepers: Optional[list[ParallelSweepers]] = None,
-    ) -> dict[Any, list]:
+    ) -> dict[PulseId, Result]:
         """Execute pulse sequences.
 
         If any sweeper is passed, the execution is performed for the different values
@@ -253,6 +263,10 @@ class Platform:
         """
         if sweepers is None:
             sweepers = []
+        if not _unique_acquisitions(sequences):
+            raise ValueError(
+                "The acquisitions' identifiers have to be unique across all sequences."
+            )
 
         options = self.settings.fill(options)
 
@@ -262,20 +276,18 @@ class Platform:
         configs = self.parameters.configs.copy()
         update_configs(configs, options.updates)
 
-        # for components that represent aux external instruments (e.g. lo) to the main control instrument
-        # set the config directly
+        # for components that represent aux external instruments (e.g. lo) to the main
+        # control instrument set the config directly
         for name, cfg in configs.items():
             if name in self.instruments:
                 self.instruments[name].setup(**cfg.model_dump(exclude={"kind"}))
 
-        results = defaultdict(list)
+        results = {}
         # pylint: disable=unsubscriptable-object
         bounds = self.parameters.configs[self._controller.bounds]
         assert isinstance(bounds, Bounds)
         for b in batch(sequences, bounds):
-            result = self._execute(b, options, configs, sweepers)
-            for serial, data in result.items():
-                results[serial].append(data)
+            results |= self._execute(b, options, configs, sweepers)
 
         return results
 
diff --git a/src/qibolab/pulses/__init__.py b/src/qibolab/pulses/__init__.py
index d8e1abcb..a26fa998 100644
--- a/src/qibolab/pulses/__init__.py
+++ b/src/qibolab/pulses/__init__.py
@@ -1,2 +1,11 @@
 from .envelope import *
-from .pulse import Acquisition, Align, Delay, Pulse, PulseLike, Readout, VirtualZ
+from .pulse import (
+    Acquisition,
+    Align,
+    Delay,
+    Pulse,
+    PulseId,
+    PulseLike,
+    Readout,
+    VirtualZ,
+)
diff --git a/src/qibolab/pulses/pulse.py b/src/qibolab/pulses/pulse.py
index 2b03b3e5..1d0b4116 100644
--- a/src/qibolab/pulses/pulse.py
+++ b/src/qibolab/pulses/pulse.py
@@ -9,10 +9,21 @@ from qibolab.serialize import Model
 
 from .envelope import Envelope, IqWaveform, Waveform
 
+__all__ = [
+    "Delay",
+    "Pulse",
+    "PulseId",
+    "PulseLike",
+    "VirtualZ",
+]
+
+PulseId = int
+"""Unique identifier for a pulse."""
+
 
 class _PulseLike(Model):
     @property
-    def id(self) -> int:
+    def id(self) -> PulseId:
         """Instruction identifier."""
         return id(self)
 
@@ -43,17 +54,17 @@ class Pulse(_PulseLike):
     """Relative phase of the pulse, in radians."""
 
     def i(self, sampling_rate: float) -> Waveform:
-        """The envelope waveform of the i component of the pulse."""
+        """Compute the envelope of the waveform i component."""
         samples = int(self.duration * sampling_rate)
         return self.amplitude * self.envelope.i(samples)
 
     def q(self, sampling_rate: float) -> Waveform:
-        """The envelope waveform of the q component of the pulse."""
+        """Compute the envelope of the waveform q component."""
         samples = int(self.duration * sampling_rate)
         return self.amplitude * self.envelope.q(samples)
 
     def envelopes(self, sampling_rate: float) -> IqWaveform:
-        """A tuple with the i and q envelope waveforms of the pulse."""
+        """Compute a tuple with the i and q envelopes."""
         return np.array([self.i(sampling_rate), self.q(sampling_rate)])
 
 
diff --git a/src/qibolab/result.py b/src/qibolab/result.py
index 675b8091..4681ba9f 100644
--- a/src/qibolab/result.py
+++ b/src/qibolab/result.py
@@ -10,6 +10,9 @@ It is assumed that the I and Q component are discriminated by the
 innermost dimension of the array.
 """
 
+Result = npt.NDArray[np.float64]
+"""An array of results."""
+
 
 def _lift(values: IQ) -> npt.NDArray:
     """Transpose the innermost dimension to the outermost."""


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index ec0d541a..ad89ecba 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -112,7 +112,7 @@ def execute(connected_platform: Platform) -> Execution:
         nshots: int = 1000,
         sweepers: Optional[list[ParallelSweepers]] = None,
         sequence: Optional[PulseSequence] = None,
-        target: Optional[tuple[int, int]] = None,
+        target: Optional[int] = None,
     ) -> npt.NDArray:
         options = ExecutionParameters(
             nshots=nshots,
@@ -145,13 +145,13 @@ def execute(connected_platform: Platform) -> Execution:
                 )
                 sweepers = [[sweeper1], [sweeper2]]
             if target is None:
-                target = (acq.id, 0)
+                target = acq.id
 
         # default target and sweepers only supported for default sequence
         assert target is not None
         assert sweepers is not None
 
         results = connected_platform.execute([sequence], options, sweepers)
-        return results[target[0]][target[1]]
+        return results[target]
 
     return wrapped
diff --git a/tests/test_dummy.py b/tests/test_dummy.py
index 5ebf96c8..b5a33277 100644
--- a/tests/test_dummy.py
+++ b/tests/test_dummy.py
@@ -60,15 +60,13 @@ def test_dummy_execute_pulse_sequence_unrolling(
     nsequences = 10
     natives = platform.natives
     sequences = []
-    sequence = PulseSequence()
-    sequence.concatenate(natives.single_qubit[0].MZ.create_sequence())
     for _ in range(nsequences):
-        sequences.append(sequence)
+        sequences.append(natives.single_qubit[0].MZ.create_sequence())
     options = ExecutionParameters(nshots=nshots, acquisition_type=acquisition)
     result = platform.execute(sequences, options)
-    assert len(next(iter(result.values()))) == nsequences
-    for r in result[0]:
+    assert len(next(iter(result.values()))) == nshots
+    for r in result.values():
         if acquisition is AcquisitionType.INTEGRATION:
-            assert r.magnitude.shape == (nshots,)
+            assert r.shape == (nshots, 2)
         if acquisition is AcquisitionType.DISCRIMINATION:
-            assert r.samples.shape == (nshots,)
+            assert r.shape == (nshots,)
diff --git a/tests/test_native.py b/tests/test_native.py
index 460dad14..dd215bc8 100644
--- a/tests/test_native.py
+++ b/tests/test_native.py
@@ -35,6 +35,8 @@ def test_fixed_sequence_factory():
     fseq2 = factory.create_sequence()
     assert fseq1 == seq
     assert fseq2 == seq
+    # while representing the same sequence, the objects are actually different
+    assert fseq1[0][1].id != fseq2[0][1].id
 
     np = "new/probe"
     fseq1.append(
diff --git a/tests/test_platform.py b/tests/test_platform.py
index d89bd099..5c978d5c 100644
--- a/tests/test_platform.py
+++ b/tests/test_platform.py
@@ -107,6 +107,21 @@ def test_platform_sampling_rate(platform):
     assert platform.sampling_rate >= 1
 
 
+def test_duplicated_acquisition():
+    """A shallow copy will duplicate the object.
+
+    This leads to non-unique identifiers across all sequences, and it's
+    then flagged as an error (since unique identifiers are assumed in
+    the return type, to avoid overwriting dict entries).
+    """
+    platform = create_platform("dummy")
+    sequence = platform.natives.single_qubit[0].MZ.create_sequence()
+
+    options = ExecutionParameters(nshots=None)
+    with pytest.raises(ValueError, match="unique"):
+        _ = platform.execute([sequence, sequence.copy()], options)
+
+
 def test_update_configs(platform):
     drive_name = "q0/drive"
     pump_name = "twpa_pump"