David/fixes split data fit #324
Conversation
for more information, see https://pre-commit.ci
…eam/qibocal into david/fixes_split_data_fit
for more information, see https://pre-commit.ci
…eam/qibocal into david/fixes_split_data_fit
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Thanks @DavidSarlle for implementing this.
Please find below a few comments, nothing critical just some minor changes.
src/qibocal/protocols/characterization/allxy/allxy_drag_pulse_tuning.py
Outdated
Show resolved
Hide resolved
| f"q{qubit} | amplitude_correction_factor: {fit.amplitude_factors[qubit]:.4f}<br>" | ||
| + f"q{qubit} | corrected_amplitude: {fit.amplitudes[qubit][0]:.4f}<br><br>" |
There was a problem hiding this comment.
Again keep capital letter at least for first word of each entry in the table.
| @@ -274,7 +274,7 @@ def _plot(data: RamseyData, fit: RamseyResults, qubit): | |||
| fitting_report | |||
| + (f"{qubit} | delta_frequency: {fit.delta_phys[qubit]:,.1f} Hz<br>") | |||
| + (f"{qubit} | drive_frequency: {fit.frequency[qubit] * 1e9} Hz<br>") | |||
| + (f"{qubit} | T2: {fit.t2[qubit]:,.0f} ns.<br><br>") | |||
| # + (f"{qubit} | T2: {fit.t2[qubit]:,.0f} ns.<br><br>") | |||
There was a problem hiding this comment.
Here I think that we can keep T2 as an estimate.
It will not be modified in the runcard.
| "qubit": ro_pulse.qubit, | ||
| } | ||
| data.add(r) | ||
| count += 1 |
There was a problem hiding this comment.
count can be removed I think
| # execute the pulse sequence | ||
| results = platform.execute_pulse_sequence(sequence) | ||
|
|
||
| # retrieve the results for every qubit | ||
| for ro_pulse in ro_pulses.values(): | ||
| z_proj = 2 * results[ro_pulse.serial].ground_state_probability - 1 | ||
| # store the results | ||
| r = { | ||
| "probability": z_proj, | ||
| "gateNumber": gateNumber, | ||
| "beta_param": params.beta_param, | ||
| "qubit": ro_pulse.qubit, | ||
| } | ||
| data.add(r) |
There was a problem hiding this comment.
Here I believe that the indentation was corrected before. Did you test it?
There was a problem hiding this comment.
I have tested and it works as expected
| f0 = guess_center | ||
| fit_res = lmfit.model.ModelResult(model=model_Q, params=guess_parameters) | ||
|
|
||
| frequency[qubit] = f0 | ||
|
|
||
| if data.power_level is PowerLevel.high: | ||
| bare_frequency[qubit] = f0 | ||
|
|
||
| amplitudes[qubit] = data.amplitude | ||
| attenuations[qubit] = data.attenuation | ||
| fitted_parameters[qubit] = fit_res.params.valuesdict() | ||
|
|
||
| if data.power_level is PowerLevel.high: | ||
| return { | ||
| "frequency": frequency, | ||
| "fitted_parameters": fitted_parameters, | ||
| "bare_frequency": bare_frequency, | ||
| "amplitude": amplitudes, | ||
| } | ||
| else: | ||
| return { | ||
| "frequency": frequency, | ||
| "fitted_parameters": fitted_parameters, | ||
| "amplitude": amplitudes, | ||
| } |
There was a problem hiding this comment.
I think that this part is not needed.
It should already be in _fit of resonator_spectroscopy.
There was a problem hiding this comment.
At the very beggining was in inside the lorentzian_fit. Sorry, I did not update after someone change it.
Fixed
There was a problem hiding this comment.
Thanks @DavidSarlle for implementing this.
Please find below a few comments, nothing critical just some minor changes.
…eam/qibocal into david/fixes_split_data_fit
for more information, see https://pre-commit.ci
This branch contins some fixes for PR #279.
Mostly fixes:
Checklist: