v1.2.0 #92
v1.2.0 #92
Conversation
Bump actions
… ExperimentSystem
…nian calculations
…n quantum simulator
…tor related examples
…ate vector plotting functionality in simulation results
…imports accordingly
…d enhance pulse scheduling with frequency and object annotations
…nction parameters and improve control signal handling
…and downsampling capability
…signal handling and parameter adjustments
…function and improve visualization with Plotly
…and visualization; update example notebook
…nsistency; update return types and downsampling logic
…ons; enhance simulation parameters and plotting
…y; remove cross_resonance_2 example notebook
…eparators for compactness
…ethods; fix parameter naming in waveform plotting
…adout and resource mapping
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/examples/simulator/8_cr_calibration.ipynb (5)
41-64: Consider documenting parameter ranges and constraints.While units are documented, it would be helpful to add comments explaining:
- Valid ranges for parameters
- Physical constraints
- Reasoning behind chosen values (e.g., why 0.333 for anharmonicity)
112-123: Add docstring and type hints for return value normalization.The
drag_pi_pulsefunction performs pulse normalization but lacks documentation explaining the normalization factor calculation.Add comprehensive documentation:
def drag_pi_pulse( duration: float, anharmonicity: float = anharmonicity, ) -> qp.Pulse: + """Generate a normalized DRAG pulse for pi rotation. + + Args: + duration: Pulse duration in ns + anharmonicity: Qubit anharmonicity in GHz + + Returns: + qp.Pulse: Normalized DRAG pulse with pi rotation area + + Note: + The pulse is normalized such that its area equals pi, + ensuring a complete pi rotation when applied. + """
242-287: Add progress tracking for long simulations.The
simulate_crfunction could benefit from progress tracking during simulation.Consider adding a progress callback:
def simulate_cr( control_drive_frequency: float, target_drive_frequency: float, cr_amplitude: float, cr_duration: float, cr_ramptime: float, cr_phase: float, crosstalk_amplitude: float = 0.0, crosstalk_phase: float = 0.0, cancel_amplitude: float = 0.0, cancel_phase: float = 0.0, echo: bool = False, control_state: str = "0", n_samples: int = 256, plot: bool = False, + progress_callback: Optional[Callable[[float], None]] = None, ) -> SimulationResult:
336-430: Consider caching intermediate results for performance.The
hamiltonian_tomographyfunction performs multiple simulations that could benefit from caching.Consider adding result caching:
from functools import lru_cache @lru_cache(maxsize=32) def _cached_simulate_cr(*args, **kwargs) -> SimulationResult: return simulate_cr(*args, **kwargs)
793-835: Add convergence monitoring to optimization.The CMA-ES optimization could benefit from convergence monitoring and logging.
Consider adding a callback:
def optimization_callback(es): """Monitor optimization progress.""" if es.countiter % 10 == 0: print(f"Iteration {es.countiter}: Best score = {es.best.f}")Then add it to the optimizer:
es = cma.CMAEvolutionStrategy( initial_guess, 1.0, { "seed": 42, "ftarget": 1e-6, "timeout": 120, + "verb_disp": 10, # Display progress every 10 iterations ... }, ) -es.optimize(objective_func) +es.optimize(objective_func, callback=optimization_callback)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/examples/simulator/8_cr_calibration.ipynb(1 hunks)
🔇 Additional comments (3)
docs/examples/simulator/8_cr_calibration.ipynb (3)
9-32: LGTM! Imports are well-organized.
The imports are appropriate for quantum simulation work and follow standard conventions. The use of autoreload is helpful for development.
73-103: LGTM! System configuration is well-structured.
The quantum system setup is clear and follows a logical structure with appropriate parameter passing.
739-784: 🛠️ Refactor suggestion
Enhance objective function robustness.
The objective_func implementation could be improved:
- Add bounds checking
- Handle simulation failures gracefully
- Add early stopping for invalid parameters
def objective_func(x: np.ndarray):
+ """Calculate infidelity with bounds checking and error handling.
+
+ Returns:
+ float: Infidelity value or infinity for invalid parameters
+ """
(
cr_phase,
cancel_amplitude,
cancel_phase,
) = x
+
+ # Check bounds
+ if not (-np.pi <= cr_phase <= np.pi and
+ -1 <= cancel_amplitude <= 1 and
+ -np.pi <= cancel_phase <= np.pi):
+ return float('inf')
+
+ try:
result_0 = simulate_cr(...)
result_1 = simulate_cr(...)
+ except Exception as e:
+ print(f"Simulation failed: {e}")
+ return float('inf')Likely invalid or redundant comment.
| "def cr_sequence(\n", | ||
| " control_drive_frequency: float,\n", | ||
| " target_drive_frequency: float,\n", | ||
| " cr_amplitude: float,\n", | ||
| " cr_duration: float,\n", | ||
| " cr_ramptime: float,\n", | ||
| " cr_phase: float,\n", | ||
| " crosstalk_amplitude: float = 0.0,\n", | ||
| " crosstalk_phase: float = 0.0,\n", | ||
| " cancel_amplitude: float = 0.0,\n", | ||
| " cancel_phase: float = 0.0,\n", | ||
| " echo: bool = False,\n", | ||
| "):\n", | ||
| " cr_waveform = qp.FlatTop(\n", | ||
| " duration=cr_duration,\n", | ||
| " amplitude=2 * np.pi * cr_amplitude,\n", | ||
| " tau=cr_ramptime,\n", | ||
| " phase_shift=cr_phase,\n", | ||
| " )\n", | ||
| " cancel_waveform = qp.FlatTop(\n", | ||
| " duration=cr_duration,\n", | ||
| " amplitude=2 * np.pi * cancel_amplitude,\n", | ||
| " tau=cr_ramptime,\n", | ||
| " phase_shift=cancel_phase,\n", | ||
| " )\n", | ||
| " crosstalk_waveform = qp.FlatTop(\n", | ||
| " duration=cr_duration,\n", | ||
| " amplitude=2 * np.pi * crosstalk_amplitude,\n", | ||
| " tau=cr_ramptime,\n", | ||
| " phase_shift=crosstalk_phase,\n", | ||
| " )\n", | ||
| " with qp.PulseSchedule(\n", | ||
| " {\n", | ||
| " \"CR\": {\n", | ||
| " \"frequency\": target_drive_frequency,\n", | ||
| " \"object\": control_qubit_label,\n", | ||
| " },\n", | ||
| " \"Crosstalk\": {\n", | ||
| " \"frequency\": target_drive_frequency,\n", | ||
| " \"object\": target_qubit_label,\n", | ||
| " },\n", | ||
| " \"Target\": {\n", | ||
| " \"frequency\": target_drive_frequency,\n", | ||
| " \"object\": target_qubit_label,\n", | ||
| " },\n", | ||
| " }\n", | ||
| " ) as cr:\n", | ||
| " cr.add(\"CR\", cr_waveform)\n", | ||
| " cr.add(\"Target\", cancel_waveform)\n", | ||
| " cr.add(\"Crosstalk\", crosstalk_waveform)\n", | ||
| "\n", | ||
| " if not echo:\n", | ||
| " return cr\n", | ||
| " else:\n", | ||
| " with qp.PulseSchedule(\n", | ||
| " {\n", | ||
| " \"Control\": {\n", | ||
| " \"frequency\": control_drive_frequency,\n", | ||
| " \"object\": control_qubit_label,\n", | ||
| " },\n", | ||
| " \"CR\": {\n", | ||
| " \"frequency\": target_drive_frequency,\n", | ||
| " \"object\": control_qubit_label,\n", | ||
| " },\n", | ||
| " \"Crosstalk\": {\n", | ||
| " \"frequency\": target_drive_frequency,\n", | ||
| " \"object\": target_qubit_label,\n", | ||
| " },\n", | ||
| " \"Target\": {\n", | ||
| " \"frequency\": target_drive_frequency,\n", | ||
| " \"object\": target_qubit_label,\n", | ||
| " },\n", | ||
| " }\n", | ||
| " ) as ecr:\n", | ||
| " ecr.call(cr)\n", | ||
| " ecr.barrier()\n", | ||
| " ecr.add(\"Control\", pi_pulse)\n", | ||
| " ecr.barrier()\n", | ||
| " ecr.call(cr.scaled(-1))\n", | ||
| " ecr.barrier()\n", | ||
| " ecr.add(\"Control\", pi_pulse)\n", | ||
| " return ecr" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid parameters.
The cr_sequence function should validate input parameters before creating pulses.
Add parameter validation:
def cr_sequence(
control_drive_frequency: float,
target_drive_frequency: float,
cr_amplitude: float,
cr_duration: float,
cr_ramptime: float,
cr_phase: float,
crosstalk_amplitude: float = 0.0,
crosstalk_phase: float = 0.0,
cancel_amplitude: float = 0.0,
cancel_phase: float = 0.0,
echo: bool = False,
):
+ """Create a cross-resonance pulse sequence.
+
+ Args: [...]
+ Raises:
+ ValueError: If parameters are invalid
+ """
+ if cr_duration <= 0 or cr_ramptime <= 0:
+ raise ValueError("Duration and ramp time must be positive")
+ if cr_ramptime * 2 > cr_duration:
+ raise ValueError("Ramp time too large compared to duration")
+ if not 0 <= cr_amplitude <= 1:
+ raise ValueError("CR amplitude must be between 0 and 1")Committable suggestion skipped: line range outside the PR's diff.
…ge for improved time handling
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/qubex/analysis/fitting.py (1)
1687-1687:⚠️ Potential issueFix mutable default argument.
Using a mutable object (numpy array) as a default argument can lead to unexpected behavior since the same array object is shared between function calls.
Apply this fix:
-def fit_rotation( - times: npt.NDArray[np.float64], - data: npt.NDArray[np.float64], - r0: npt.NDArray[np.float64] = np.array([0, 0, 1]), +def fit_rotation( + times: npt.NDArray[np.float64], + data: npt.NDArray[np.float64], + r0: npt.NDArray[np.float64] | None = None,Then initialize the default value within the function:
if r0 is None: r0 = np.array([0, 0, 1])🧰 Tools
🪛 Ruff (0.8.2)
1687-1687: Do not perform function call
np.arrayin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
🧹 Nitpick comments (1)
src/qubex/analysis/fitting.py (1)
260-359: Add error handling for polynomial fitting.While the function handles root finding errors well, it should also handle potential errors during polynomial fitting.
Apply this improvement:
- popt = np.polyfit(x, y, degree) - fun = np.poly1d(popt) - y_fit = fun(x) + try: + popt = np.polyfit(x, y, degree) + fun = np.poly1d(popt) + y_fit = fun(x) + except (ValueError, np.linalg.LinAlgError) as e: + print(f"Failed to fit polynomial for {target}: {str(e)}") + return { + "popt": None, + "fun": None, + "root": None, + "fig": None, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/qubex/analysis/fitting.py(35 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/qubex/analysis/fitting.py
1687-1687: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (4)
src/qubex/analysis/fitting.py (4)
214-238: LGTM! Well-implemented square root Lorentzian function.
The implementation is mathematically correct and includes proper type hints and documentation.
1684-1928: LGTM! Well-implemented 3D rotation fitting function.
The implementation includes:
- Proper error handling and input validation
- Efficient parameter estimation using FFT
- Comprehensive 2D and 3D visualizations
- Clear documentation and type hints
🧰 Tools
🪛 Ruff (0.8.2)
1687-1687: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
521-521: LGTM! Improved return types for better maintainability.
The change from tuples to dictionaries makes the return values more explicit and easier to extend in the future.
Also applies to: 618-618, 753-753, 1249-1249, 1472-1472
615-615: LGTM! Improved consistency in axis labels.
The standardization of y-axis titles from "Amplitude (arb. units)" to "Signal (arb. unit)" improves the consistency of the visualization outputs.
Also applies to: 750-750, 1246-1246, 1356-1356
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/qubex/analysis/fitting.py (1)
1687-1687:⚠️ Potential issueFix mutable default argument.
Using a mutable object (numpy array) as a default argument can lead to unexpected behavior since the same array object is shared between function calls.
Apply this fix:
-def fit_rotation( - times: npt.NDArray[np.float64], - data: npt.NDArray[np.float64], - r0: npt.NDArray[np.float64] = np.array([0, 0, 1]), +def fit_rotation( + times: npt.NDArray[np.float64], + data: npt.NDArray[np.float64], + r0: npt.NDArray[np.float64] | None = None,Then initialize the default value within the function:
if r0 is None: r0 = np.array([0, 0, 1])🧰 Tools
🪛 Ruff (0.8.2)
1687-1687: Do not perform function call
np.arrayin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
🧹 Nitpick comments (1)
src/qubex/analysis/fitting.py (1)
260-359: Add error handling for polynomial fitting.While the implementation is good, it should handle potential fitting failures gracefully, similar to other fitting functions in the module.
Apply this improvement:
- popt = np.polyfit(x, y, degree) - fun = np.poly1d(popt) - y_fit = fun(x) + try: + popt = np.polyfit(x, y, degree) + fun = np.poly1d(popt) + y_fit = fun(x) + except (ValueError, np.linalg.LinAlgError) as e: + print(f"Failed to fit polynomial: {str(e)}") + return { + "popt": None, + "fun": None, + "root": None, + "fig": None, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/qubex/analysis/fitting.py(35 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/qubex/analysis/fitting.py
1687-1687: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (4)
src/qubex/analysis/fitting.py (4)
214-238: LGTM! Well-implemented square root Lorentzian function.
The implementation is mathematically correct and follows best practices with proper type hints and documentation.
488-495: LGTM! Good plot enhancement.
The added frequency annotation improves the plot's readability and provides important information directly on the visualization.
1684-1929: LGTM! Excellent implementation of 3D rotation fitting.
The implementation is mathematically sound with:
- Proper handling of rotation matrices
- Good initial parameter estimation using FFT
- Well-designed 2D and 3D visualizations
🧰 Tools
🪛 Ruff (0.8.2)
1687-1687: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
521-521: LGTM! Good improvement in return types.
The change from tuples to dictionaries makes the return values more explicit and self-documenting, improving code maintainability and readability.
Also applies to: 538-539, 618-618, 649-650, 753-754, 784-785, 1472-1472, 1493-1494
…lse for clarity; update related logic in Experiment class
…rameters for enhanced clarity and functionality
…on fitting options; add z-values support for signal processing
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/qubex/analysis/fitting.py (1)
1684-1694:⚠️ Potential issueFix mutable default argument.
Using a mutable object (numpy array) as a default argument can lead to unexpected behavior since the same array object is shared between function calls.
Apply this fix:
def fit_rotation( times: npt.NDArray[np.float64], data: npt.NDArray[np.float64], - r0: npt.NDArray[np.float64] = np.array([0, 0, 1]), + r0: npt.NDArray[np.float64] | None = None, p0=None, bounds=None, plot: bool = True, title: str = "State evolution", xlabel: str = "Time (ns)", ylabel: str = "Expectation value", ) -> dict:Then initialize the default value within the function:
if r0 is None: r0 = np.array([0, 0, 1])🧰 Tools
🪛 Ruff (0.8.2)
1687-1687: Do not perform function call
np.arrayin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
src/qubex/analysis/visualization.py (1)
18-53: 🛠️ Refactor suggestionAdditional improvements needed for the save_figure_image function
While the previous review already covered input validation, there are additional improvements needed:
- Add return type hint (-> None)
- Validate format parameter matches the file extension
- Use atomic file operations to handle race conditions
def save_figure_image( fig: go.Figure, name: str = "image", *, images_dir: str = "./images", format: Literal["png", "svg", "jpeg", "webp"] = "png", width: int = 600, height: int = 300, scale: int = 3, -): +) -> None: + if not format in ["png", "svg", "jpeg", "webp"]: + raise ValueError(f"Unsupported format: {format}") + if not os.path.exists(images_dir): - os.makedirs(images_dir) + os.makedirs(images_dir, exist_ok=True) counter = 1 current_date = datetime.datetime.now().strftime("%Y%m%d") - file_path = os.path.join( - images_dir, - f"{current_date}_{name}_{counter}.{format}", - ) - - while os.path.exists(file_path): + while True: + file_path = os.path.join( + images_dir, + f"{current_date}_{name}_{counter}.{format}", + ) + try: + # Try to create file exclusively + fd = os.open(file_path, os.O_CREAT | os.O_EXCL) + os.close(fd) + break + except FileExistsError: + counter += 1 + continueThe previous review's input validation suggestions are still valid and should be implemented alongside these changes.
🧹 Nitpick comments (2)
src/qubex/analysis/fitting.py (1)
260-359: Add input validation for polynomial degree.While the implementation is solid, it should validate that the polynomial degree is non-negative and less than the number of data points to prevent potential fitting issues.
def fit_polynomial( *, target: str, x: npt.NDArray[np.float64], y: npt.NDArray[np.float64], degree: int, plot: bool = True, title: str = "Polynomial fit", xaxis_title: str = "x", yaxis_title: str = "y", xaxis_type: Literal["linear", "log"] = "linear", yaxis_type: Literal["linear", "log"] = "linear", ) -> dict: + if degree < 0: + raise ValueError("Polynomial degree must be non-negative") + if degree >= len(x): + raise ValueError("Polynomial degree must be less than the number of data points")src/qubex/analysis/visualization.py (1)
302-306: Consider parameterizing the image dimensionsWhile the save_image integration is good, the hardcoded width/height values should be parameterized for flexibility.
def scatter_iq_data( data: Mapping[str, IQArray], *, save_image: bool = False, + image_width: int = 500, + image_height: int = 400, title: str = "I/Q plane", xlabel: str = "In-phase (arb. unit)", ylabel: str = "Quadrature (arb. unit)", ) -> None: # ... existing code ... if save_image: save_figure_image( fig, name="iq_data", - width=500, - height=400, + width=image_width, + height=image_height, )Also applies to: 355-361
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/qubex/analysis/fitting.py(35 hunks)src/qubex/analysis/visualization.py(10 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/qubex/analysis/fitting.py
1687-1687: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (9)
src/qubex/analysis/fitting.py (4)
214-238: LGTM! Well-documented implementation of the square root Lorentzian function.
The function is correctly implemented and includes comprehensive docstrings with clear parameter descriptions.
1695-1929: LGTM! Comprehensive implementation of 3D rotation fitting.
The implementation includes:
- Correct mathematical formulation of rotation matrices
- FFT-based parameter estimation
- Comprehensive visualization with both 2D and 3D plots
- Clear error handling and input validation
521-521: LGTM! Improved API consistency with dictionary returns.
The change from tuples/floats to dictionaries as return types is a good improvement because it:
- Makes the API more consistent across all fitting functions
- Makes the return values self-documenting
- Allows for future extensibility without breaking changes
Also applies to: 538-539, 618-618, 649-650, 753-754, 784-785, 1472-1472, 1493-1494
615-615: LGTM! Consistent terminology in axis labels.
The change from "Amplitude (arb. units)" to "Signal (arb. unit)" makes the terminology more consistent and accurate across all plotting functions.
Also applies to: 750-750, 1246-1246, 1356-1356
src/qubex/analysis/visualization.py (5)
139-139: LGTM: Consistent terminology update
The ylabel change maintains consistency with other plotting functions in the module.
213-217: LGTM: Improved function signature and consistent terminology
Good improvements:
- Making arguments keyword-only with
*improves function call safety - ylabel change maintains consistency across the module
246-249: LGTM: Consistent improvements
The changes align with the module-wide improvements in function signatures and terminology.
376-377: LGTM: Enhanced visualization styling
Good improvements:
- Consistent terminology in axis labels
- Enhanced text styling with better visibility and readability
Also applies to: 412-425
168-175: LGTM: Improved function naming and flexibility
The renaming to plot_bloch_vectors better reflects the quantum mechanical context, and the new mode parameter adds useful visualization options.
Let's verify the impact of the function rename:
✅ Verification successful
Function rename has been properly propagated throughout the codebase
The search results show that:
- No references to the old
plot_state_vectorsname exist in the codebase - The new
plot_bloch_vectorsfunction is:- Defined in
src/qubex/analysis/visualization.py - Exported in
src/qubex/analysis/__init__.py - Imported and used correctly in
src/qubex/simulator/quantum_simulator.pyandsrc/qubex/experiment/experiment.py
- Defined in
All references have been updated to use the new name, and there are no lingering references to the old function name that would cause issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to the old function name
rg "plot_state_vectors" --type py
Length of output: 33
Script:
#!/bin/bash
# Let's search for both old and new function names to understand the usage
rg "plot_(state|bloch)_vectors" --type py -A 2
Length of output: 1463
Script:
#!/bin/bash
# Let's also check git history to see the changes
git log -p --no-merges -G "plot_state_vectors" -- "*.py"
Length of output: 0
Script:
#!/bin/bash
# Let's check for any test files that might use this function
fd "test.*\.py$" --exec grep -l "plot_.*vectors"
Length of output: 50
Script:
#!/bin/bash
# Let's check for any imports of this function
ast-grep --pattern 'from $_ import plot_state_vectors'
Length of output: 56
Script:
#!/bin/bash
# Check for any imports of the new function name
ast-grep --pattern 'from $_ import plot_bloch_vectors'
Length of output: 142
…tles and labels for clarity
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/qubex/analysis/fitting.py (1)
260-359: Consider handling polynomial fitting edge cases
The function elegantly handles scenarios without real roots in range by returningnp.nan. However, you might want to capture more detail about fitting failures, such as returning an error message in the dictionary instead of merely printing it, to maintain a consistent structured return for all outcomes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/qubex/analysis/fitting.py(35 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/qubex/analysis/fitting.py
1687-1687: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (1)
src/qubex/analysis/fitting.py (1)
1687-1687: Fix mutable default argument
This closely matches a previous comment regarding using np.array as a default argument. The same array instance can be shared across calls, leading to unintended side effects.
-def fit_rotation(
- times: npt.NDArray[np.float64],
- data: npt.NDArray[np.float64],
- r0: npt.NDArray[np.float64] = np.array([0, 0, 1]),
- ...
+def fit_rotation(
+ times: npt.NDArray[np.float64],
+ data: npt.NDArray[np.float64],
+ r0: npt.NDArray[np.float64] | None = None,
+ ...
):
+ if r0 is None:
+ r0 = np.array([0, 0, 1])🧰 Tools
🪛 Ruff (0.8.2)
1687-1687: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
| @@ -322,7 +449,7 @@ def fit_rabi( | |||
| # ) | |||
| except RuntimeError: | |||
| print(f"Failed to fit the data for {target}.") | |||
| return RabiParam(target, 0.0, 0.0, 0.0, 0.0, noise, angle) | |||
| return RabiParam(target, np.nan, np.nan, np.nan, np.nan, noise, angle) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify return type for consistent library API
While returning a RabiParam object is optimal for object-oriented usage, almost all other fitting functions return a dictionary. If you want to keep the library interface uniform, consider switching to a dictionary, or conversely migrate the other functions to return a data class.
-def fit_rabi(...) -> RabiParam:
+def fit_rabi(...) -> dict:
...
- return RabiParam(
- target=target,
- amplitude=amplitude,
- ...
- )
+ return {
+ "target": target,
+ "amplitude": amplitude,
+ "frequency": frequency,
+ "phase": phase,
+ "offset": offset,
+ "noise": noise,
+ "angle": angle,
+ }Committable suggestion skipped: line range outside the PR's diff.
| def func_sqrt_lorentzian( | ||
| f: npt.NDArray[np.float64], | ||
| A: float, | ||
| f0: float, | ||
| Omega: float, | ||
| C: float, | ||
| ) -> npt.NDArray[np.float64]: | ||
| """ | ||
| Calculate a square root Lorentzian function with given parameters. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| f : npt.NDArray[np.float64] | ||
| Frequency points for the function evaluation. | ||
| A : float | ||
| Amplitude of the square root Lorentzian function. | ||
| f0 : float | ||
| Central frequency of the square root Lorentzian function. | ||
| Omega : float | ||
| Width of the square root Lorentzian function. | ||
| C : float | ||
| Vertical offset of the square root Lorentzian function. | ||
| """ | ||
| return A / np.sqrt(1 + ((f - f0) / Omega) ** 2) + C | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Add parameter validation for Omega in sqrt-lorentzian function
The codebase shows that func_sqrt_lorentzian is used in curve fitting without explicit bounds or validation for Omega. While curve_fit can accept bounds, they aren't being set for the sqrt-lorentzian fit. To prevent potential domain issues, you should add a validation check for Omega > 0 in the function implementation.
- In
src/qubex/analysis/fitting.py, add parameter validation tofunc_sqrt_lorentzian:
def func_sqrt_lorentzian(f, A, f0, Omega, C):
if Omega <= 0:
raise ValueError("Omega must be positive")
return A / np.sqrt(1 + ((f - f0) / Omega) ** 2) + C🔗 Analysis chain
Confirm domain validity for sqrt-lorentzian
While the implementation looks correct, consider verifying that Omega > 0 to avoid potential domain issues (e.g., division by zero or taking the square root of negative values) if the input parameters or data differ from your assumptions.
You can run the following script to check whether func_sqrt_lorentzian is only called with valid, positive Omega parameters throughout the codebase:
Let me gather more information about the parameter bounds and validation.
Let me check one more thing to see if there are any parameter validations or bounds specifically for the sqrt-lorentzian fitting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for usage of func_sqrt_lorentzian to see if there is any clamp or check for Omega
rg -A 10 "func_sqrt_lorentzian"
Length of output: 1789
Script:
#!/bin/bash
# Search for bounds definition and parameter validation
rg -B 5 -A 10 "bounds=" src/qubex/analysis/fitting.py
Length of output: 7083
Script:
#!/bin/bash
# Search for the sqrt lorentzian fit function implementation
rg -B 10 -A 20 "def fit_sqrt_lorentzian" src/qubex/analysis/fitting.py
Length of output: 864
…ndling; update data normalization in Experiment class
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/qubex/analysis/fitting.py (2)
260-359: Add error handling for polynomial fitting.While the function handles root finding errors well, it should also handle potential errors from
np.polyfit(). Consider wrapping the fitting in a try-except block:- popt = np.polyfit(x, y, degree) - fun = np.poly1d(popt) - y_fit = fun(x) + try: + popt = np.polyfit(x, y, degree) + fun = np.poly1d(popt) + y_fit = fun(x) + except (ValueError, np.linalg.LinAlgError) as e: + print(f"Failed to fit polynomial: {str(e)}") + return { + "popt": None, + "fun": None, + "root": None, + "fig": None, + }
1770-1780: Add validation for FFT-based parameter estimation.The initial parameter estimation using FFT could fail if the input data is too short or contains no periodic signal.
Add validation:
if p0 is None: N = len(times) + if N < 2: + raise ValueError("At least 2 data points required for parameter estimation") dt = times[1] - times[0] + if dt <= 0: + raise ValueError("Time steps must be positive") F = np.array(fft(data[:, 2])) f = np.array(fftfreq(N, dt)[1 : N // 2]) + if len(f) == 0: + raise ValueError("Cannot estimate frequency from the given data") i = np.argmax(np.abs(F[1 : N // 2]))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/qubex/analysis/fitting.py(34 hunks)src/qubex/experiment/experiment_result.py(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/qubex/experiment/experiment_result.py
🧰 Additional context used
🪛 Ruff (0.8.2)
src/qubex/analysis/fitting.py
1693-1693: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (4)
src/qubex/analysis/fitting.py (4)
214-238: Add parameter validation for Omega.
The function needs validation to ensure Omega > 0 to prevent domain errors when calculating the square root.
452-452: LGTM! Improved consistency and plot readability.
The changes enhance the function by:
- Making the return type consistent with other fitting functions
- Adding clear frequency annotation to the plot
Also applies to: 488-495
Line range hint 521-602: LGTM! Enhanced error reporting and visualization.
The changes improve the function by:
- Providing comprehensive error information in the return dictionary
- Adding clear annotations to the plot
- Maintaining consistency with other fitting functions
1869-1926: LGTM! Well-implemented 3D quantum state visualization.
The visualization is comprehensive and includes:
- Data points and fit line in 3D space
- Semi-transparent Bloch sphere
- Appropriate axis labels and scaling
| def fit_rotation( | ||
| times: npt.NDArray[np.float64], | ||
| data: npt.NDArray[np.float64], | ||
| r0: npt.NDArray[np.float64] = np.array([0, 0, 1]), |
There was a problem hiding this comment.
Fix mutable default argument.
Using a mutable object (numpy array) as a default argument can lead to unexpected behavior since the same array object is shared between function calls.
Apply this fix:
-def fit_rotation(
- times: npt.NDArray[np.float64],
- data: npt.NDArray[np.float64],
- r0: npt.NDArray[np.float64] = np.array([0, 0, 1]),
+def fit_rotation(
+ times: npt.NDArray[np.float64],
+ data: npt.NDArray[np.float64],
+ r0: npt.NDArray[np.float64] | None = None,Then initialize the default value within the function:
if r0 is None:
r0 = np.array([0, 0, 1])🧰 Tools
🪛 Ruff (0.8.2)
1693-1693: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
…oved flexibility; add x180 parameter support in Experiment class
…ing logic for pulse sequences
…rove pulse scheduling for control qubits
…er placement in Experiment class
…_range defaults for improved clarity and flexibility in Experiment class
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/qubex/pulse/pulse_schedule.py (5)
Line range hint
19-50: LGTM! Consider enhancing the docstring.The new targets parameter implementation correctly handles both list and dictionary inputs, allowing for flexible target metadata storage.
Consider updating the docstring to include an example of dictionary-based initialization:
# Example with dictionary initialization targets = { "Q01": {"frequency": 5.0, "object": "qubit1"}, "Q02": {"frequency": 5.2, "object": "qubit2"} } ps = PulseSchedule(targets)
233-264: Enhance docstrings for transformation methods.The new transformation methods are well-implemented but would benefit from more detailed documentation.
Add parameter descriptions and return type documentation:
def scaled(self, scale: float) -> PulseSchedule: """ Returns a scaled pulse schedule. Parameters ---------- scale : float The scaling factor to apply to all pulses. Returns ------- PulseSchedule A new pulse schedule with scaled amplitudes. """
Line range hint
278-397: LGTM! Consider caching downsampled data.The plot method improvements with downsampling and metadata annotations are well implemented. However, for frequently plotted schedules, consider caching the downsampled data.
def _get_downsampled_data(self, data: npt.NDArray, n_samples: int) -> npt.NDArray: """Cache and return downsampled data.""" cache_key = (data.tobytes(), n_samples) if not hasattr(self, '_downsample_cache'): self._downsample_cache = {} if cache_key not in self._downsample_cache: self._downsample_cache[cache_key] = self._downsample(data, n_samples) return self._downsample_cache[cache_key]
497-505: Add return type hint to _downsample method.The downsampling implementation is efficient but would benefit from complete type hints.
@staticmethod def _downsample( data: npt.NDArray, n_max_points: int, - ) -> npt.NDArray: + ) -> npt.NDArray[np.float64]:
Line range hint
1-505: Consider implementing serialization for pulse schedules.The PulseSchedule class has evolved into a robust implementation for quantum pulse control. Consider adding serialization capabilities to save and load pulse schedules, which would be valuable for experiment reproducibility.
Key aspects to consider:
- JSON serialization for metadata (targets, frequencies, objects)
- Efficient binary format for pulse data (e.g., HDF5)
- Version tracking for backward compatibility
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/qubex/pulse/pulse_schedule.py(13 hunks)
🔇 Additional comments (3)
src/qubex/pulse/pulse_schedule.py (3)
55-90: LGTM! Well-structured property implementations.
The new properties provide clean access to target attributes with good encapsulation and clear documentation.
Line range hint 464-486: LGTM! Clean implementation of pulse range retrieval.
The method correctly uses the new labels property and maintains clear type hints.
Line range hint 141-192: Verify backward compatibility of barrier method changes.
The barrier method now defaults to using targets.keys() instead of the previous targets list. While the implementation is correct, this change might affect existing code.
✅ Verification successful
Let me analyze the barrier method usage in the codebase to verify backward compatibility.
No backward compatibility issues with barrier method changes
Based on the test cases and usage patterns found in the codebase:
- The barrier method is used in two ways:
- Without arguments (
barrier()): Used to synchronize all targets - With specific targets (
barrier(["Q00", "Q01"])): Used to synchronize specific targets
- Without arguments (
Both these usage patterns are preserved in the new implementation:
- When called without arguments,
targets or list(self.targets.keys())correctly defaults to all targets - When called with specific targets, the provided list is used directly
The change from using the targets list to targets.keys() maintains the same behavior because both approaches result in the same set of target labels being synchronized, as verified by the passing test cases test_barrier() and test_specific_barrier().
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing barrier method calls that might be affected
rg -A 2 "barrier\(" --type py
Length of output: 7864
Script:
#!/bin/bash
# Let's check the test file more closely to understand the behavior
cat tests/pulse/test_pulse_schedule.py
Length of output: 3698
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/pulse/test_blank.py (2)
3-3: Consider removing or retaining the blank line based on team conventions.This empty line can improve readability by visually separating imports from business logic. If your coding guidelines discourage isolated blank lines, feel free to remove it.
Line range hint
8-24: Strengthen array comparisons forpulse.values.When verifying that
pulse.valuesholds zeros, consider usingnp.array_equal(pulse.values, expected_array)or a specialized NumPy testing helper likenp.testing.assert_array_equalfor more reliable comparisons and clearer error messages.tests/pulse/test_flat_top.py (1)
Line range hint
16-27: Comprehensive initialization test.
- Testing with valid parameters and comparing
pulse.valuesto a known expected list is ideal for confirming correct amplitude shaping.- Consider also asserting the pulse’s amplitude property or other derived attributes for an even broader coverage of correctness.
tests/pulse/test_gaussian.py (1)
21-22: Verifying parameter initialization and waveform calculation.
- Checking specified parameters, including
sigmaandbeta, is crucial for Gaussian waveform generation.- Approximating expected complex values confirms wave correctness.
tests/pulse/test_pulse.py (1)
Line range hint
47-62: Typo in function name and thorough zero-padding tests.
- There’s an extra “d” in
test_paddded; please rename it totest_paddedto maintain consistency.- The zero-padding checks (left and right) are well-executed. Verifying the new values array is commendable.
-def test_paddded(): +def test_padded(): """Pulse should be padded with zeros."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
tests/pulse/test_blank.py(1 hunks)tests/pulse/test_drag.py(1 hunks)tests/pulse/test_flat_top.py(1 hunks)tests/pulse/test_gauss.py(0 hunks)tests/pulse/test_gaussian.py(2 hunks)tests/pulse/test_phase_shift.py(1 hunks)tests/pulse/test_pulse.py(1 hunks)tests/pulse/test_pulse_sequence.py(1 hunks)tests/pulse/test_raised_cosine.py(2 hunks)tests/pulse/test_rect.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/pulse/test_gauss.py
✅ Files skipped from review due to trivial changes (1)
- tests/pulse/test_pulse_sequence.py
🔇 Additional comments (29)
tests/pulse/test_phase_shift.py (1)
3-3: Good addition of PhaseShift, Pulse, and PulseSequence imports.
The explicit import of these classes increases test clarity and ensures that the PhaseShift class is being properly utilized in the suite.
tests/pulse/test_drag.py (2)
3-3: New import statements look good.
Importing Drag and Pulse together is appropriate and clarifies the relationship between the two classes for testing.
Line range hint 30-36: Excellent edge case coverage with zero duration testing.
Including the test_zero_duration method to validate that an empty array of complex128 values is generated helps ensure robust coverage for corner cases.
tests/pulse/test_rect.py (1)
3-3: Accurate import for Rect class.
This aligns with the updated pulse architecture, and the class is properly tested below.
tests/pulse/test_raised_cosine.py (5)
4-4: Import statement updated to reference RaisedCosine correctly.
Replacing any old references (e.g., DragCos) with the new RaisedCosine class ensures tests remain in sync with code changes.
10-11: Clear inheritance test for RaisedCosine.
Testing that RaisedCosine is indeed a subclass of Pulse verifies the class hierarchy.
15-17: Appropriate error handling for missing duration parameter.
Raising a TypeError for missing arguments is consistent with Python’s built-in behavior, ensuring clarity for developers.
21-22: Comprehensive initialization test.
Validating RaisedCosine(duration=5 * dt, amplitude=1, beta=1) ensures correct parameter handling and value generation.
36-37: Zero duration coverage is thorough.
Ensuring that a RaisedCosine pulse with zero duration yields an empty array extends test robustness for edge cases.
tests/pulse/test_flat_top.py (5)
3-3: No issues with the new import spacing.
The blank line helps to visually separate third-party imports from internal module imports, improving readability.
Line range hint 7-10: Test case naming is descriptive and purposeful.
The test_inheritance function clearly focuses on verifying class hierarchy, ensuring FlatTop inherits from Pulse. Good coverage for confirming inheritance relationships.
Line range hint 12-14: Proper exception testing.
The test_empty_init function appropriately checks that calling FlatTop() without required initialization parameters raises a TypeError. This helps to maintain robust parameter validation.
Line range hint 29-34: Zero-duration edge case handled gracefully.
Having a test that ensures FlatTop returns an empty array with zero duration is a good approach to confirm there's no unexpected behavior in boundary conditions.
Line range hint 36-41: Proper error handling for invalid duration.
- Verifying that a
ValueErroris raised whenduration <= 2 * tauensures mathematical integrity of the pulse waveform. - The clear error message enhances transparency for any caller misconfigurations.
tests/pulse/test_gaussian.py (5)
3-4: Sensible import for new Gaussian class.
The updated import statement for Gaussian aligns with the recent refactoring. This is consistent with the project’s pulse module structure.
10-11: Accurate test for inheritance.
The test_inheritance function confirms that Gaussian inherits from Pulse, helping ensure expected hierarchy is maintained.
15-17: Correct exception test for missing arguments.
Raising a TypeError when duration is not specified fulfills Python’s constructor argument requirements and prevents silent misconfigurations.
36-37: Solid coverage for edge-case with zero duration.
Ensuring Gaussian results in an empty array for zero duration helps confirm no unexpected math or indexing errors occur in boundary conditions.
43-45: Ensuring invalid parameter raises the correct error.
A ValueError when sigma=0 is vital to avoid division or exponential anomalies in Gaussian amplitude computations.
tests/pulse/test_pulse.py (10)
3-3: No issues with the new import and sampling period usage.
The addition of Waveform and the recognition of Pulse.SAMPLING_PERIOD is appropriate for testing.
Line range hint 7-10: Inheritance test looks solid.
The test correctly asserts that Pulse is a subclass of Waveform.
Line range hint 13-16: Good negative test coverage for Pulse initialization.
You correctly ensure a TypeError is raised when no parameters are provided, preventing unintended usage.
Line range hint 19-23: Validation of empty list initialization.
This test confirms behavioral correctness for edge cases (no values). All assertions are relevant.
Line range hint 26-36: Comprehensive check on real, imaginary, and complex values.
Excellent coverage ensuring that Pulse([0, -0.5, +0.5j]) sets all derived attributes (real, imag, abs, angle) correctly.
Line range hint 39-44: Copy method testing is concise.
The test verifies that copy produces a distinct instance with identical values, adhering to expected immutable semantics.
Line range hint 65-69: Scaling test is straightforward and effective.
The approach of verifying scaled values via pytest.approx is appropriate and ensures relative floating-point accuracy.
Line range hint 72-84: Proper detuning validation.
Applying a complex exponential shift for each time step is correct. Testing intermediate steps with pytest.approx ensures numerical stability.
Line range hint 87-93: Phase-shifting test covers negative real/imag transitions.
The test ensures each value is phase-shifted correctly and remains distinct from the original Pulse.
Line range hint 96-101: Repetition logic verified.
Repeating the pulse multiple times is effectively tested, confirming that the updated values array meets expectations.
…enhanced description
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
README.md (2)
Line range hint
6-9: Review the pip version requirement.The requirement for pip 24.0 seems incorrect as this version hasn't been released yet. Consider:
- Specifying a minimum pip version instead (e.g., "pip >= 21.0")
- Or removing the specific pip version requirement if it's not strictly necessary
Line range hint
1-39: Add version information and highlight new features.Since this is part of the v1.2.0 release:
- Consider adding a version badge or current version number
- Include a "What's New" or "Changelog" section highlighting the key features mentioned in the PR (quantum simulation improvements, experiment management enhancements, etc.)
🧹 Nitpick comments (2)
README.md (2)
1-4: Consider enhancing the introduction with more specific details.The introduction provides a good overview but could be more helpful to users by:
- Listing specific supported quantum hardware platforms
- Adding links to documentation for key features (T1/T2 experiments, gate calibration, etc.)
- Including a brief code example demonstrating basic usage
36-39: Enhance the usage section with more details.Consider improving the usage section by:
- Adding a quick-start example
- Providing a brief description of each example in the examples folder
- Including links to specific example notebooks mentioned in the AI summary (e.g.,
0_basic_usage.ipynb,1_frequency_inspection.ipynb)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
| ### qubex with backend (only for Linux) | ||
|
|
||
| ```bash | ||
| pip install "qubex[backend] @ git+https://github.com/amachino/qubex.git" | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify backend installation requirements and features.
The backend installation section would benefit from:
- Explaining what additional features the backend provides
- Listing specific Linux dependencies or requirements
- Clarifying why it's Linux-only
| ```bash | ||
| git clone git+https://github.com/amachino/qubex.git | ||
| cd qubex | ||
| pip install -e . |
There was a problem hiding this comment.
Fix the git clone command syntax.
The git clone command contains an incorrect URL format.
Apply this correction:
-git clone git+https://github.com/amachino/qubex.git
+git clone https://github.com/amachino/qubex.git
Summary by CodeRabbit
New Features
PulseScheduleclass to support a more structured representation of targets.QuantumSystemclass.Experimentclass methods to simplify parameter handling and introduced new methods for state tomography and population dynamics.StateManagerclass to improve configurability and state handling.Cliffordclass.CrossResonanceclass for managing cross-resonance pulse schedules.Bug Fixes
Documentation
Style
Tests
PulseScheduleclass and other functionalities.Chores
pyproject.tomland versioning inversion.py.