Rabi Refactor - #1561
Open
lballerio wants to merge 22 commits into
Open
Conversation
Contributor
Author
|
check #1108 for crosstalk calibration references. |
lballerio
force-pushed
the
rabi_refactor
branch
2 times, most recently
from
June 30, 2026 14:49
20bc18e to
4a9e81a
Compare
lballerio
marked this pull request as ready for review
June 30, 2026 14:49
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1561 +/- ##
==========================================
- Coverage 91.41% 90.39% -1.02%
==========================================
Files 148 151 +3
Lines 11844 12049 +205
==========================================
+ Hits 10827 10892 +65
- Misses 1017 1157 +140
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the Rabi protocol implementations to support driving a target qubit using a different physical drive line, while also consolidating shared Rabi data/processing/acquisition logic and modernizing calibration crosstalk handling.
Changes:
- Add
drive_linessupport across Rabi parameter models and acquisition routines to enable cross-drive (target ≠ drive line) experiments. - Split and reuse common Rabi logic via new
parent_classes.py,acquisition.py, andprocessing.py, updating protocol modules accordingly. - Replace raw crosstalk matrices with a
Matrixmodel (flux + microwave) and update related protocols/tests to use the new API.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_protocols.py | Updates tests to new Rabi data class names used by extract_rabi. |
| tests/test_calibration.py | Adapts serialization tests to the new Matrix-backed crosstalk matrices and new Calibration(...) constructor usage. |
| tests/calibration_scripts/rx_calibration.py | Removes chi2 / small-correction guards in the calibration script. |
| src/qibocal/update.py | Removes the legacy crosstalk_matrix updater helper (call sites updated elsewhere). |
| src/qibocal/protocols/utils.py | Tightens typing for frequency/period estimation helpers. |
| src/qibocal/protocols/rabi/utils.py | Refactors/retains shared Rabi utilities; adds typing and sequence helpers (now partially duplicated with new acquisition module). |
| src/qibocal/protocols/rabi/processing.py | New shared post-processing module (fit functions, plotting, update logic including microwave crosstalk update). |
| src/qibocal/protocols/rabi/parent_classes.py | New shared dataclasses for Rabi parameters/results/data, including drive_lines plumbing. |
| src/qibocal/protocols/rabi/length.py | Migrates length-classification protocol to shared acquisition/processing and drive_lines support. |
| src/qibocal/protocols/rabi/length_signal.py | Migrates length-signal protocol to shared acquisition/processing and drive_lines support. |
| src/qibocal/protocols/rabi/length_frequency.py | Migrates length+frequency classification chevron to shared acquisition/processing and drive_lines support. |
| src/qibocal/protocols/rabi/length_frequency_signal.py | Migrates length+frequency signal chevron to shared acquisition/processing and drive_lines support. |
| src/qibocal/protocols/rabi/ef.py | Refactors EF Rabi to new shared parameter/data types and adds cross-drive support. |
| src/qibocal/protocols/rabi/amplitude.py | Migrates amplitude-classification protocol to shared acquisition/processing and drive_lines support. |
| src/qibocal/protocols/rabi/amplitude_signal.py | Migrates amplitude-signal protocol to shared acquisition/processing and drive_lines support. |
| src/qibocal/protocols/rabi/amplitude_frequency.py | Migrates amplitude+frequency classification chevron to shared acquisition/processing and drive_lines support. |
| src/qibocal/protocols/rabi/amplitude_frequency_signal.py | Migrates amplitude+frequency signal chevron to shared acquisition/processing and drive_lines support. |
| src/qibocal/protocols/rabi/acquisition.py | New shared acquisition module implementing drive-line validation and sequence building. |
| src/qibocal/protocols/flux_dependence/resonator_flux_dependence.py | Switches to new flux_crosstalk_matrix[...] access API. |
| src/qibocal/protocols/flux_dependence/qubit_flux_dependence.py | Updates flux crosstalk update path to the new matrix API. |
| src/qibocal/protocols/flux_dependence/qubit_crosstalk.py | Updates acquisition/update logic to the new flux crosstalk matrix API. |
| src/qibocal/protocols/flux_dependence/flux_amplitude_frequency.py | Updates crosstalk update path to the new matrix API. |
| src/qibocal/calibration/serialize.py | Makes deserializers accept already-materialized matrix/ndarray inputs. |
| src/qibocal/calibration/calibration.py | Introduces Matrix model and adds flux_crosstalk_matrix + microwave_crosstalk_matrix with validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lballerio
marked this pull request as draft
July 1, 2026 07:43
lballerio
marked this pull request as ready for review
July 1, 2026 07:43
Comment on lines
+107
to
+109
| data = RabiAmplitudeFreqClassificationData( | ||
| drive_lines=drive_lines, durations=durations, rx90=params.rx90 | ||
| ) |
Comment on lines
+109
to
+111
| data = RabiAmplitudeFreqSignalData( | ||
| drive_lines=drive_lines, durations=durations, rx90=params.rx90 | ||
| ) |
Comment on lines
+218
to
+219
| if self.flux_crosstalk_matrix.matrix.shape[0] != self.nqubits: | ||
| raise ValueError("Drive crosstalk matrix must have as many rows as qubits.") |
RoyStegeman
reviewed
Jul 1, 2026
alecandido
reviewed
Jul 8, 2026
alecandido
left a comment
alecandido
left a comment
Member
There was a problem hiding this comment.
For the time being, just about serialization
lballerio
marked this pull request as draft
July 16, 2026 14:21
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
lballerio
marked this pull request as ready for review
July 17, 2026 14:24
lballerio
marked this pull request as draft
July 21, 2026 14:16
lballerio
marked this pull request as ready for review
July 21, 2026 14:22
lballerio
marked this pull request as draft
July 21, 2026 15:06
lballerio
marked this pull request as ready for review
July 22, 2026 06:02
alecandido
reviewed
Aug 25, 2026
alecandido
left a comment
alecandido
left a comment
Member
There was a problem hiding this comment.
Ok, I'm kind at half of it. But this PR is definitely way too big, let me cut the first review here.
Member
There was a problem hiding this comment.
Ok, I understand the point, but parent_classes.py sounds a bit inaccurate.
On the generic side, structures.py could be a candidate. But data.py is also an option - though it can be slightly misleading, since we have *Data classes.
Or even just classes.py would be more accurate on its own (some of the classes are directly used).
| :noindex: | ||
|
|
||
| We must note that all Rabi protocols takes as an optional input also `drive_lines`, which indicates the lines to use for each qubit. | ||
| If this parameter is set it has to be of the same length of `targets` list and there must not be repeated elements in these two lists, otherwise an error is raised. |
Member
There was a problem hiding this comment.
Not so clear. I would recommend adding an example of a good parameters' choice, and a bad one.
| """T2 hanh echo [ns].""" | ||
| rb_fidelity: Measure | None = None | ||
| """Standard rb pulse fidelity.""" | ||
| rabi_ampl_oscillation: dict[Literal["signal", "classification"], float] = Field( |
Member
There was a problem hiding this comment.
You could give a name to this Literal[...] type, since we may use in multiple places.
An alternative option could be using a StrEnum. But, until we want to use the Enum to simplify some kind of consumption (iterating it, dynamically checking if it is part of the set, and similar) a type alias of the Literal is more than enough.
Let's avoid overcomplicating things, until we have a use case (which lifts the "over" part).
Comment on lines
+187
to
+188
| if self.flux_crosstalk_matrix is None or self.flux_crosstalk_matrix.size == 0: | ||
| self.flux_crosstalk_matrix = np.eye(self.nqubits) |
Member
There was a problem hiding this comment.
I'm not sure whether it is a good idea, but in principle you do not need this to be part of a custom validator, and you do not need None as a placeholder default.
It looks like default-setting callables in Pydantic are allowed to depend on previous fields https://pydantic.dev/docs/validation/latest/concepts/fields/#default-values
Thus, you could replace this with:
flux_crosstalk_matrix: NdArray = Field(default=lambda data: np.eye(len(data["single_qubits"])))
Comment on lines
+199
to
+209
| If the matrix is missing or empty, initialize it as a complex-valued | ||
| matrix of shape (nqubits, nqubits) filled with infinity. If provided, | ||
| verify the matrix shape is correct and convert its dtype to complex. | ||
| """ | ||
| if ( | ||
| self.microwave_crosstalk_matrix is None | ||
| or self.microwave_crosstalk_matrix.size == 0 | ||
| ): | ||
| self.microwave_crosstalk_matrix = np.full( | ||
| (self.nqubits, self.nqubits), np.inf, dtype=complex | ||
| ) |
Member
There was a problem hiding this comment.
I'm not sure I understand why you want to initialize it with infinity.
We are not even measuring it in decibels - which is fine, since floats can already do a great job at representing exponents.
Thus, why not np.zeros()?
Comment on lines
+10
to
+11
| pulse_duration_range: tuple[float, float, float] | None = None | ||
| """Pulse duration [ns] range.""" |
Member
There was a problem hiding this comment.
Suggested change
| pulse_duration_range: tuple[float, float, float] | None = None | |
| """Pulse duration [ns] range.""" | |
| duration: tuple[float, float, float] | None = None | |
| """Pulse duration [ns] range.""" |
This is already unambiguous.
Comment on lines
+50
to
+51
| ampl_range: tuple[float, float, float] | None = None | ||
| """Pulse minimum amplitude [a.u] range.""" |
Member
There was a problem hiding this comment.
As above:
Suggested change
| ampl_range: tuple[float, float, float] | None = None | |
| """Pulse minimum amplitude [a.u] range.""" | |
| amplitude: tuple[float, float, float] | None = None | |
| """Pulse minimum amplitude [a.u] range.""" |
Comment on lines
+84
to
+85
| freq_range: tuple[int, int, int] | None = None | ||
| """Frequency range as an offset.""" |
Member
There was a problem hiding this comment.
Suggested change
| freq_range: tuple[int, int, int] | None = None | |
| """Frequency range as an offset.""" | |
| frequency: tuple[int, int, int] | None = None | |
| """Frequency range as an offset.""" |
Comment on lines
+113
to
+114
| freq_range: tuple[int, int, int] | None = None | ||
| """Frequency range as an offset.""" |
Member
There was a problem hiding this comment.
Suggested change
| freq_range: tuple[int, int, int] | None = None | |
| """Frequency range as an offset.""" | |
| frequency: tuple[int, int, int] | None = None | |
| """Frequency range as an offset.""" |
Comment on lines
+109
to
+111
| @dataclass | ||
| class RabiAmplitudeFrequencyParameters(RabiAmplitudeParameters): | ||
| """RabiChevronAmplitude runcard inputs.""" |
Member
There was a problem hiding this comment.
Not sure how well does it work with @dataclass, but usually, to reduce this kind of duplication, one uses mixins:
@dataclass
class FrequencyMixin:
freq_range: tuple[int, int, int] | None = None
"""Frequency range as an offset."""
min_freq: int | None = None
"""Minimum frequency as an offset."""
max_freq: int | None = None
"""Maximum frequency as an offset."""
step_freq: int | None = None
"""Frequency to use as step for the scan."""
@property
def frequency_range(self) -> tuple[float, float, float]:
"""
Return a tuple with the duration times of the pulses.
"""
if self.freq_range is None:
return (self.min_freq, self.max_freq, self.step_freq)
return self.freq_range
def __post_init__(self):
super().__post_init__()
if any([f is None for f in self.frequency_range]):
raise ValueError("Valid frequency offset range not inserted.")
class RabiLengthFrequencyParameters(RabiLengthParameters, FrequencyMixin):
"""RabiChevronLength runcard inputs."""
class RabiAmplitudeFrequencyParameters(RabiAmplitudeParameters, FrequencyMixin):
"""RabiChevronAmplitude runcard inputs."""
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes the following changes:
rabifolder:a.
utils.pyhas now been splitted in 3 modules:acquisition.py, which contains all rabi sequence functions,processing.py, which contains functions for plotting and fitting andparent_classes.py, which defines the commonData,ParametersandResultsclasses.b. all rabi experiments refactored accordingly
range-likeconvention for input parameters.drive_linesoptional input, a list of qubits which indicates the list of physical lines to use for each qubit; also some quality checks of the input are performed:a. if defined,
drive_linesandtargetsmust have the same lengthb. the must not be repeated elements in the union of
drive_linesandtargets.microwave_crosstalk_matrixfield incalibration.pywith also a validator forCalibrationclass andgetadsetfunction for this matrix.rabi_amplitudeexperiment now also saves inmicrowave_crosstalk_matrixthe fitted amplitude from the signal.Calibrationclass now is also saved the fitted amplitude of non-crosstalk Rabi experiment for each qubit; this is used later to updatemicrowave_crosstalk_matrix.update.pyand propagated this change influx_dependencefolder.microwave_cancellation_phase.pymodule for calibrating mw crosstalk phase.testsfolder.