Add sweeper support for flipping_amplitude - #1508
Conversation
flipping_amplitude
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## flipping_amplitude #1508 +/- ##
======================================================
- Coverage 93.76% 93.71% -0.06%
======================================================
Files 137 137
Lines 10799 10803 +4
======================================================
- Hits 10126 10124 -2
- Misses 673 679 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
lballerio
left a comment
There was a problem hiding this comment.
Thanks @sorewachigauyo for this implementation.
As I said, I think there is still something we can refactor and simplify, but we can postpone this discussion after we merge this PR on PR #1504.
As you mentioned here and in PR #1507, in your implementation we are recycling the same pulse, which I am getting convinced might be the more practical solution since we are sweeping the same way all the pulses in the experiment, so we might actually recycle the pulse so we don't have a huge list of pulses which differ only for the UUID we sweep in the same way...
Btw did you test it?
I am simply waiting for @jevillegasd review.
| for flips, sequence in zip(flips_range, sequences): | ||
| for qubit, sweeper in zip(targets, parallel_sweepers): | ||
| acq_channel = platform.qubits[qubit].acquisition | ||
| assert acq_channel is not None | ||
| ro_pulse = list(sequence.channel(acq_channel))[-1] | ||
| assert isinstance(ro_pulse, Readout) | ||
| prob = results[ro_pulse.id] | ||
| error = np.sqrt(prob * (1 - prob) / params.nshots) | ||
| native_amp = data.pulse_amplitudes[qubit] | ||
| data.register_qubit( | ||
| FlippingAmplitudeType, | ||
| qubit, | ||
| { | ||
| "flips": np.array([flips]), | ||
| "amplitude": np.array([native_amp + delta_amp]), | ||
| "prob": np.array([prob]), | ||
| "error": np.array([error]), | ||
| }, | ||
| ) | ||
| prob_array = results[ro_pulse.id] | ||
| assert len(prob_array) == len(sweeper.values) | ||
| for amp, prob in zip(sweeper.values, prob_array): | ||
| error = np.sqrt(prob * (1 - prob) / params.nshots) | ||
| data.register_qubit( | ||
| FlippingAmplitudeType, | ||
| qubit, | ||
| { | ||
| "flips": np.array([flips]), | ||
| "amplitude": np.array([amp]), | ||
| "prob": np.array([prob]), | ||
| "error": np.array([error]), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
maybe can be simplified, but we can do it in PR #1504.
| # First two pulses are the pi/2 pulse and unused alignment and the last pulse is a readout alignment | ||
| # So we add every pulse in-between | ||
| # Technically we can reduce the number of pulses by filtering by unique UUID | ||
| pulses_to_sweep[qubit].extend( | ||
| list(seq.channel(platform.qubits[qubit].drive))[2:-1] | ||
| ) |
There was a problem hiding this comment.
I am not an expert on this experiment, but should we also sweep on the initial RX90 pulse amplitude?
There was a problem hiding this comment.
Personally, I don't think we have to.
This particular experiment is looking for the minimum variance between adding multiple
I think the initial state affects the original experiment more under certain conditions, as that decides under/over-rotation fitting.
There was a problem hiding this comment.
I see your point, yeah probably your right, then probably won't make such a difference.
On my driver implementation, sweeping/playing multiple pulses with the same UUID is not an issue. My concern from the other PR is if that was an issue with QM/Qblox, which I don't have in my lab to test. If this isn't a problem, then yeah, we should go ahead with the recycling, because in the current implementation (prior to #1507), each call of
Yeah, it was actually in the other PR, but here is a new test |
|
Hi @sorewachigauyo, I just tried to run this implementation on qblox and actually the sweeper is too big and does not fit, it raises an error. |
|
Hi @lballerio Its a bit more extreme rewrite that doesn't reuse the existing sequence generation and should make qibolab happier If this doesn't work, then I'll probably change this into a separate PR for sweepers |
|
Hi @sorewachigauyo I tried your new branch and the results I think a re very satifying!
I also worked on this sweeper implementation and ended up with https://github.com/qiboteam/qibocal/tree/flipping_amplitude_sweeper_lballerio.
if you want to have a look, but I think you're implementation is more than fine...maybe we should move the PR on that branch. |
|
Yeah, its the same UUID/pulse object for the entire experiment Okay, good to see that it works with Qblox |
|
perfect, yeah I like your implementation! |
lballerio
left a comment
There was a problem hiding this comment.
tested with Qblox on TII qw5q_platinum QPU, I think is good
…cation. This is still however unable to run for long pulse durations in the spin lock pulse.



As discussed in PR #1504, an implementation of the routine with amplitude sweeper
This probably has to be modified based on how 42aea44 goes and if #1507 gets merged due to
which I'm not super happy about, but it works for now