Add calibration result plotting functions #234
Conversation
|
Missing:
|
Co-authored-by: TheoLaudatQM <[email protected]>
Co-authored-by: TheoLaudatQM <[email protected]>
Co-authored-by: TheoLaudatQM <[email protected]>
There was a problem hiding this comment.
I started the review and many comments apply to all functions, namely:
- instead of an empty
return, I think that raising an error makes more sense since it indicates to the user that something went wrong and it also indicates what. - In general it would be nice to add comments explaining what the different parts of the code do so that we (CS) can maintain it later.
- It looks like there are a lot of sections that are common to the different functions, isn't it possible to break them down into small private functions (
def _function_name()) that can be called in the main functions. It will make the code more readable and easier to test and maintain.
I even think that using a class could make sense here, what do you think?
paulQM
force-pushed
the
octave-tools-plotter
branch
from
47de2b1 to
5cc1468
Compare
|
|
||
| ## show_lo_leakage_calibration_result() | ||
| Plot the LO leakage calibration data. | ||
| The produced plot shows the LO leakage signal as a function of the `I_0` and `Q_O` dc offsets for an initial coarse scan and a finer zoom-in scan on the minima. A third plot shows the fit error of the fine scan. |
There was a problem hiding this comment.
minimum instead of minimum since there is only one
|
|
||
| ### Outputs: | ||
|
|
||
|  |
There was a problem hiding this comment.
I would place the dc gain and phase under the "current results" instead for better visibility
|
|
||
| ### Outputs: | ||
|
|
||
|  |
There was a problem hiding this comment.
I would also specify that the displayed powers correspond to the powers measured by the OPX after up and down conversion and may differ significantly from the power outputted by the Octave, or something along these lines
|
|
||
| plotter = CalibrationResultPlotter(calibration_output) | ||
|
|
||
| plotter.show_lo_leakage_calibration_result() |
There was a problem hiding this comment.
I think it would make sense to return the fig object so that people can modify it and save it easily for instance, what do you think?
|
|
||
| ## show_image_rejection_calibration_result() | ||
| Plot the image sideband calibration data. | ||
| The produced plot shows the image sideband signal as a function of the `dc_gain` and `dc_phase` parameters for an initial coarse scan and a finer zoom-in scan on the minima. A third plot shows the fit error of the fine scan. |
There was a problem hiding this comment.
minimum instead of minimum since there is only one
| This method generates a series of subplots that visualize the coarse and fine scans of the IF image calibration | ||
| and returns the achieved image suppression in dB. |
There was a problem hiding this comment.
I would also specify that the displayed powers correspond to the powers measured by the OPX after up and down conversion and may differ significantly from the power outputted by the Octave, or something along these lines
A well as the meaning of dB here "Here the returned value is with respect to before the calibration (so taking into account the previously load calibration parameters), or without any parameters?"
| If the LO frequency is not given, the first LO frequency in the data is used. | ||
| If the IF frequency is not given, the first IF frequency in the data is used. |
There was a problem hiding this comment.
The Lo and IF are not part of the input arguments, are they?
| if_freq (Optional[float]): The IF frequency in Hz. If not provided, the first IF frequency in data is used. | ||
|
|
||
| Returns: | ||
| float: The reduction of Image sideband power before vs after calibration in dB units. |
There was a problem hiding this comment.
I would again specify in dB with respect to what
| If the LO frequency is not given, the first LO frequency in the data is used. | ||
| If the IF frequency is not given, the first IF frequency in the data is used. | ||
|
|
There was a problem hiding this comment.
The Lo and IF are not part of the input arguments, are they?
| If the IF frequency is not given, the first IF frequency in the data is used. | ||
|
|
||
| Returns: | ||
| float: The reduction of LO leakage power before vs after calibration in dB units. |
There was a problem hiding this comment.
I would again specify in dB with respect to what
Added tools to plot the mixer calibration results.
show_lo_result()andshow_if_result()plot the calibration data of the LO leakage and image sideband as well as the chosen calibration parameters:I_0,Q_O,dc_gainanddc_phase.