Adding PCA fit for rabi signal - #1678
Draft
lballerio wants to merge 14 commits into
Draft
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed runtime-breaking issues in the updated tests and in length_frequency_signal (dtype/register_qubit mismatch and plotting/data-shape problems) that must be fixed before the PR can be safely merged.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a PCA-based analysis of measured IQ quadratures for Rabi signal experiments, aiming to improve fit robustness (by fitting along principal components rather than the raw magnitude) and to enhance reporting plots with IQ-plane and PC-projection views.
Changes:
- Add PCA-based IQ-plane plotting utilities and update Rabi signal plotting to include IQ plane + principal/residual component projections.
- Refactor frequency/period estimation helpers to be more array-friendly and adjust several call sites to convert NumPy outputs to JSON-serializable Python types.
- Update Rabi fit implementations and add new test fixtures for the signal-based Rabi amplitude case.
File summaries
| File | Description |
|---|---|
| tests/test_fit_functions.py | Updates Rabi fit test logic (currently introduces a broken conditional and mismatched call signatures). |
| tests/rabi_fit_data/rabi_amplitude_signal-100/results.json | Adds new expected results fixture for Rabi amplitude signal PCA-based fitting. |
| tests/rabi_fit_data/rabi_amplitude_signal-100/data.json | Adds new fixture metadata for the Rabi amplitude signal dataset. |
| src/qibocal/update.py | Adjusts drive duration typing to accept float durations. |
| src/qibocal/protocols/zz_interaction/jazz.py | Converts Quinn–Fernandes output to JSON-friendly .tolist(). |
| src/qibocal/protocols/utils.py | Refactors Quinn–Fernandes implementation and adds plot_iq_pca helper. |
| src/qibocal/protocols/two_qubit_interaction/utils.py | Converts period guess to JSON-friendly .tolist(). |
| src/qibocal/protocols/two_qubit_interaction/cross_resonance/cross_resonance_processing.py | Uses median of Quinn–Fernandes output and converts to .tolist(). |
| src/qibocal/protocols/ramsey/processing.py | Converts Quinn–Fernandes output to .tolist(). |
| src/qibocal/protocols/rabi/utils.py | Introduces PCA-based plotting for signal-based Rabi routines and refactors initial guess/fitting helpers. |
| src/qibocal/protocols/rabi/length.py | Updates length-fit flow to use unnormalized x and updated fit helpers. |
| src/qibocal/protocols/rabi/length_signal.py | Switches stored signal representation from magnitude/phase to raw i/q and fits along PCA principal axis; adds plot wrapper. |
| src/qibocal/protocols/rabi/length_frequency.py | Updates to new fit helper signatures (removes old scaling params). |
| src/qibocal/protocols/rabi/length_frequency_signal.py | Switches 2D acquisition/fit/plot to IQ+PCA approach and adds 1D plot embedding (currently has dtype/register_qubit mismatches). |
| src/qibocal/protocols/rabi/amplitude.py | Updates to new fit helper signatures (removes old scaling params). |
| src/qibocal/protocols/rabi/amplitude_signal.py | Switches stored signal representation from magnitude/phase to raw i/q and fits along PCA principal axis. |
| src/qibocal/protocols/rabi/amplitude_frequency.py | Updates to new fit helper signatures (removes old scaling params). |
| src/qibocal/protocols/rabi/amplitude_frequency_signal.py | Switches 2D acquisition/fit/plot to IQ+PCA approach and adds 1D plot embedding (heatmap currently flattens PC matrix). |
| src/qibocal/protocols/drag/drag.py | Converts period guess to JSON-friendly .tolist(). |
Review details
- Files reviewed: 19/20 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+186
to
+190
| qubit=qubit, | ||
| freq=freq_sweepers[qubit].values, | ||
| lens=len_sweeper.values, | ||
| signal=magnitude(result), | ||
| phase=phase(result), | ||
| signal=result[..., 0], | ||
| phase=result[..., 1], |
Comment on lines
+311
to
+313
| fitted_data = data.return_row_data(selected_frequency, target) | ||
| rabi1d_figure, rabi1d_report = plot(fitted_data, target, fit, data.rx90) | ||
| fitting_report += rabi1d_report |
Comment on lines
263
to
269
| fig.add_trace( | ||
| go.Heatmap( | ||
| x=amplitudes, | ||
| y=frequencies, | ||
| z=qubit_data.phase, | ||
| colorbar_x=1.01, | ||
| z=pc_matrix.ravel(), | ||
| colorbar_x=1.0, | ||
| ), |
Comment on lines
273
to
279
| fig.add_trace( | ||
| go.Heatmap( | ||
| x=durations, | ||
| y=frequencies, | ||
| z=qubit_data.phase, | ||
| colorbar_x=1.01, | ||
| z=pc_matrix.ravel(), | ||
| colorbar_x=1.0, | ||
| ), |
Comment on lines
+76
to
+78
| if "signal in str_sub": | ||
| signal_flag = True | ||
| elif "freq" in str_sub: |
Comment on lines
+76
to
+78
| if "signal in str_sub": | ||
| signal_flag = True | ||
| elif "freq" in str_sub: |
There was a problem hiding this comment.
🟡 Changes recommended
Several paths currently fail at runtime or return calibration values in normalized rather than physical units.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
tests/test_fit_functions.py:78
- This condition tests a non-empty string literal, so it is always true; moreover, that branch initializes only
signal_flag, leavingxandsignalundefined before they are used below. The Rabi fit test therefore fails before exercising any fit. Test the substring itself and initialize the raw inputs (and update the later call arguments to match the new fit API).
if "signal in str_sub":
signal_flag = True
elif "freq" in str_sub:
src/qibocal/protocols/rabi/length_frequency_signal.py:70
- The dtype now defines
iandq, butregister_qubitstill writesdata["signal"]anddata["phase"]at lines 96-97. Every length-frequency signal acquisition will raiseValueError: no field of name signalbefore fitting. Rename the registration arguments and assignments toi/q.
("i", np.float64),
("q", np.float64),
src/qibocal/protocols/rabi/length_frequency_signal.py:123
- This returns
RabiLengthFreqSignalData, whose sweep field is namedlen, but the newly reused 1-Dplot()detects a Rabi-length dataset and accessesqubit_data.length. Consequently, plotting any successful frequency fit raisesAttributeError. Convert the selected row toRabiLengthSignalDatawith fieldslength,i, andqbefore passing it to the shared plotter.
return RabiLengthFreqSignalData(
rx90=self.rx90, amplitudes=self.amplitudes, data={qubit: selected_freq_data}
)
- Files reviewed: 19/20 changed files
- Comments generated: 6
- Review effort level: Balanced
Comment on lines
57
to
+58
| RabiAmpSignalType = np.dtype( | ||
| [("amp", np.float64), ("signal", np.float64), ("phase", np.float64)] | ||
| [("amp", np.float64), ("i", np.float64), ("q", np.float64)] |
Comment on lines
+389
to
+393
| popt = np.asarray(popt).tolist() | ||
| perr = np.sqrt(np.diag(perr)).tolist() | ||
|
|
||
| pi_pulse_parameter = popt[2] / 2 * period_correction_factor(phase=popt[3]) | ||
| return popt, perr.tolist(), pi_pulse_parameter | ||
| return popt, perr, pi_pulse_parameter |
Comment on lines
396
to
+400
| def fit_amplitude_function( | ||
| x, y, guess, sigma=None, signal=True, x_limits=(None, None), y_limits=(None, None) | ||
| x, | ||
| y, | ||
| guess, | ||
| sigma=None, |
Comment on lines
+415
to
+420
| popt = np.asarray(popt).tolist() | ||
| perr = np.sqrt(np.diag(perr)).tolist() | ||
|
|
||
| pi_pulse_parameter = popt[2] / 2 * period_correction_factor(phase=popt[3]) | ||
|
|
||
| return popt, perr.tolist(), pi_pulse_parameter | ||
| return popt, perr, pi_pulse_parameter |
Comment on lines
+64
to
+66
| if not np.isscalar(period): | ||
| zeros = np.zeros_like(period) | ||
| phase_guess = np.zeros_like(period) * phase_guess |
Comment on lines
+1316
to
+1321
| i_plot = np.linspace(np.min(i), np.max(i), 200) | ||
| scatters.extend( | ||
| [ | ||
| go.Scatter( | ||
| x=i_plot, | ||
| y=a[1] / a[0] * (i_plot - centroid_x) + centroid_y, |
There was a problem hiding this comment.
Updated in commit 3c01d71c: principal axes are now finite segments centered at the PCA centroid and derived directly from each component vector, including vertical components.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: lballerio <130075247+lballerio@users.noreply.github.com>
lballerio
force-pushed
the
rabi_pca_fits
branch
from
September 3, 2026 10:49
d3f6151 to
91a8555
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1678 +/- ##
==========================================
+ Coverage 90.47% 90.48% +0.01%
==========================================
Files 151 151
Lines 12225 12225
==========================================
+ Hits 11060 11062 +2
+ Misses 1165 1163 -2
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.
🟡 Changes recommended
The amplitude-frequency signal heatmap supplies flattened coordinates and values with incompatible two-dimensional heatmap semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 21/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.
In this PR I am introducing a PCA analysis of the IQ components measured during rabi signal experiments.
This method is preventing the fitted data to be excessively distorted in the
magnitudecomponent of the measured signal (what's being fitted inmain), improving the overall quality of the fit.Also the plotting in the report has changed, including also the IQ-plane plot and the plots along all principal components: