Open JakeKitchen opened 1 month ago
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪ |
🏅 Score: 92 |
🧪 No relevant tests |
🔒 No security concerns identified |
⚡ Recommended focus areas for review Performance Optimization Verify that the optimizations in the `_wigner_discretized_iterative` function, including precomputation of exponential terms and square roots, and matrix reuse, actually improve performance without introducing numerical instability. Code Refactoring Review the changes in the `shape` method of the `Tensor` class to ensure that the new implementation correctly handles all possible input scenarios and maintains the expected behavior. |
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.
Explore these optional code suggestions:
Category | Suggestion | Score |
Performance |
Use NumPy's vectorized operations to improve performance in grid creation___ **Consider using NumPy's vectorized operations instead of nested loops in themake_grid function to improve performance. This can be achieved by using np.meshgrid to create the coordinate matrices.** [mrmustard/physics/wigner.py [29-48]](https://github.com/XanaduAI/MrMustard/pull/504/files#diff-133da5e283683fe9f83202a7fc05ff614f84cdf71682d7622a9a5ae5c3deb6e7R29-R48) ```diff @njit def make_grid(q_vec: np.ndarray, p_vec: np.ndarray, hbar: float): r"""Returns two coordinate matrices `Q` and `P` from coordinate vectors `q_vec` and `p_vec`, along with the grid over which Wigner functions can be discretized. """ - n_q = q_vec.size - n_p = p_vec.size - Q = np.empty((n_q, n_p), dtype=np.float64) - P = np.empty((n_q, n_p), dtype=np.float64) + Q, P = np.meshgrid(q_vec, p_vec, indexing='ij') sqrt_factor = 1.0 / np.sqrt(2.0 * hbar) - - for i in range(n_q): - q = q_vec[i] - for j in range(n_p): - p = p_vec[j] - Q[i, j] = q - P[i, j] = p - grid = (Q + 1j * P) * sqrt_factor return Q, P, grid ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: The suggestion to use NumPy's `meshgrid` function instead of nested loops significantly improves performance by leveraging vectorized operations, which is a well-known optimization technique in numerical computing. | 9 |
Use in-place operations and preallocate arrays to reduce memory allocations and improve performance___ **Consider using in-place operations and preallocating arrays to reduce memoryallocations and improve performance in the _wigner_discretized_iterative function.**
[mrmustard/physics/wigner.py [155-169]](https://github.com/XanaduAI/MrMustard/pull/504/files#diff-133da5e283683fe9f83202a7fc05ff614f84cdf71682d7622a9a5ae5c3deb6e7R155-R169)
```diff
@njit
def _wigner_discretized_iterative(rho, q_vec, p_vec, hbar): # pragma: no cover
"""Optimized iterative computation of the Wigner function."""
cutoff = len(rho)
Q, P, grid = make_grid(q_vec, p_vec, hbar)
Wmat = np.zeros((2, cutoff) + grid.shape, dtype=np.complex128)
# Precompute the exponential term to avoid recalculating it.
- exp_grid = np.exp(-2.0 * np.abs(grid) ** 2) / np.pi
+ np.exp(-2.0 * np.abs(grid) ** 2, out=Wmat[0, 0])
+ Wmat[0, 0] /= np.pi
- # Initialize Wmat and W with the |0><0| component.
- Wmat[0, 0] = exp_grid
- W = rho[0, 0].real * Wmat[0, 0].real
+ # Initialize W with the |0><0| component.
+ W = np.real(rho[0, 0] * Wmat[0, 0])
# Precompute square roots to avoid repetitive calculations.
- sqrt_n = np.array([np.sqrt(n) for n in range(cutoff)], dtype=np.float64)
+ sqrt_n = np.sqrt(np.arange(cutoff, dtype=np.float64))
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 9Why: The suggestion to use in-place operations and preallocate arrays is a strong optimization strategy that reduces memory allocations and enhances performance, particularly in computationally intensive functions like `_wigner_discretized_iterative`. | 9 | |
Utilize NumPy's broadcasting to simplify computations and improve performance___ **Consider using NumPy's broadcasting capabilities to simplify the computation ofWmat elements and improve performance in the _wigner_discretized_iterative function.**
[mrmustard/physics/wigner.py [172-183]](https://github.com/XanaduAI/MrMustard/pull/504/files#diff-133da5e283683fe9f83202a7fc05ff614f84cdf71682d7622a9a5ae5c3deb6e7R172-R183)
```diff
-for n in range(1, cutoff):
- Wmat[0, n] = (2.0 * grid * Wmat[0, n - 1]) / sqrt_n[n]
- W += 2 * np.real(rho[0, n] * Wmat[0, n])
+# Compute Wmat[0] for all n at once
+Wmat[0, 1:] = (2.0 * grid * Wmat[0, :-1].T / sqrt_n[1:]).T
+W += 2 * np.real(np.sum(rho[0, 1:] * Wmat[0, 1:], axis=0))
# Compute the remaining coefficients and accumulate the Wigner function.
for m in range(1, cutoff):
- Wmat[1, m] = (2 * np.conj(grid) * Wmat[0, m] - sqrt_n[m] * Wmat[0, m - 1]) / sqrt_n[m]
- W += rho[m, m].real * Wmat[1, m].real
+ Wmat[1, m:] = ((2 * np.conj(grid) * Wmat[0, m:].T - sqrt_n[m] * Wmat[0, m-1:].T) / sqrt_n[m:]).T
+ W += rho[m, m].real * Wmat[1, m].real + 2 * np.real(np.sum(rho[m, m+1:] * Wmat[1, m+1:], axis=0))
- for n in range(m + 1, cutoff):
- Wmat[1, n] = (2 * grid * Wmat[1, n - 1] - sqrt_n[m] * Wmat[0, n - 1]) / sqrt_n[n]
- W += 2 * np.real(rho[m, n] * Wmat[1, n])
-
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 8Why: This suggestion effectively uses NumPy's broadcasting capabilities to simplify and optimize the computation of `Wmat` elements, which can lead to performance improvements in the `_wigner_discretized_iterative` function. | 8 | |
✅ Use tuples instead of lists for storing modes to improve performance and reduce memory usage___Suggestion Impact:The commit changed the initialization of modes in the AdjointView class to use keys() directly, which returns a view object similar to a tuple, instead of converting them to lists. This aligns with the suggestion to use more efficient data structures. code diff: ```diff + modes_in_ket=self._original.input.bra.keys(), + modes_out_ket=self._original.output.bra.keys(), + modes_in_bra=self._original.input.ket.keys(), + modes_out_bra=self._original.output.ket.keys(), ) def value(self, shape: tuple[int]): @@ -397,7 +405,12 @@ ComplexTensor: the unitary matrix in Fock representation """ # converting the given shape into a shape for the original tensor - shape_in_ket, shape_out_ket, shape_in_bra, shape_out_bra = self._original.unpack_shape(shape) + ( + shape_in_ket, + shape_out_ket, + shape_in_bra, + shape_out_bra, + ) = self._original.unpack_shape(shape) shape_ret = shape_in_bra + shape_out_bra + shape_in_ket + shape_out_ket ret = math.conj(math.astensor(self._original.value(shape_ret))) @@ -413,10 +426,10 @@ self._original = tensor super().__init__( name=self._original.name, - modes_in_ket=list(self._original.output.ket.keys()), - modes_out_ket=list(self._original.input.ket.keys()), - modes_in_bra=list(self._original.output.bra.keys()), - modes_out_bra=list(self._original.input.bra.keys()), + modes_in_ket=self._original.output.ket.keys(), + modes_out_ket=self._original.input.ket.keys(), + modes_in_bra=self._original.output.bra.keys(), + modes_out_bra=self._original.input.bra.keys(), ```storing the modes in the Tensor class initialization to improve performance and reduce memory usage.** [mrmustard/math/tensor_networks/tensors.py [186-191]](https://github.com/XanaduAI/MrMustard/pull/504/files#diff-c37f10f883a6d5dca46cceb10fc281d137fc78db80c19ed8362a1858aeb7d614R186-R191) ```diff def __init__( self, name: str, - modes_in_ket: list[int] | None = None, - modes_out_ket: list[int] | None = None, - modes_in_bra: list[int] | None = None, - modes_out_bra: list[int] | None = None, + modes_in_ket: tuple[int, ...] | None = None, + modes_out_ket: tuple[int, ...] | None = None, + modes_in_bra: tuple[int, ...] | None = None, + modes_out_bra: tuple[int, ...] | None = None, ): self._name = name self._update_modes(modes_in_ket, modes_out_ket, modes_in_bra, modes_out_bra) def _update_modes( self, - modes_in_ket: list[int] | None = None, - modes_out_ket: list[int] | None = None, - modes_in_bra: list[int] | None = None, - modes_out_bra: list[int] | None = None, + modes_in_ket: tuple[int, ...] | None = None, + modes_out_ket: tuple[int, ...] | None = None, + modes_in_bra: tuple[int, ...] | None = None, + modes_out_bra: tuple[int, ...] | None = None, ): - self._modes_in_ket = modes_in_ket if modes_in_ket else [] - self._modes_out_ket = modes_out_ket if modes_out_ket else [] - self._modes_in_bra = modes_in_bra if modes_in_bra else [] - self._modes_out_bra = modes_out_bra if modes_out_bra else [] + self._modes_in_ket = modes_in_ket if modes_in_ket else () + self._modes_out_ket = modes_out_ket if modes_out_ket else () + self._modes_in_bra = modes_in_bra if modes_in_bra else () + self._modes_out_bra = modes_out_bra if modes_out_bra else () ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Changing from lists to tuples for storing modes can improve performance and reduce memory usage due to the immutability and fixed size of tuples, making this a reasonable suggestion for optimization. However, the impact might be less significant compared to other suggestions. | 7 |
💡 Need additional feedback ? start a PR chat
Attention: Patch coverage is 7.14286%
with 13 lines
in your changes missing coverage. Please review.
Project coverage is 89.61%. Comparing base (
3a1d8bb
) to head (e55355f
).
Files with missing lines | Patch % | Lines |
---|---|---|
mrmustard/physics/wigner.py | 7.14% | 13 Missing :warning: |
User description
Optimized Wigner Function Iterative Computation
Context:
The
_wigner_discretized_iterative
function calculates the Wigner function iteratively, which is essential for simulating quantum states in continuous variable quantum mechanics. The original implementation had redundant operations, recalculations, and excessive memory allocations, leading to performance bottlenecks. Themake_grid
function was also optimized with@njit
to further enhance performance. This update improves both mathematical efficiency and memory management.Description of the Change:
make_grid
Function:make_grid
function was updated to use@njit
to accelerate its performance.Q
andP
coordinate matrices are optimized to reduce overhead.sqrt_factor
is used to avoid redundant calculations inside nested loops.@njit
, the function is now compliant with Numba’s JIT compilation, allowing faster and more efficient execution.Benefits:
Improved Mathematical Efficiency:
Better Performance:
make_grid
: Using@njit
inmake_grid
improves both speed and memory efficiency, enabling seamless integration with the rest of the optimized functions.Numba Compliance:
@njit
constraints, avoiding common type-related issues.Possible Drawbacks:
Related GitHub Issues:
NA
PR Type
enhancement, bug_fix
Description
Wire
class intensors.py
by usingfield
for attribute initialization and improved dictionary comprehensions for better performance.make_grid
function inwigner.py
by preallocating arrays and optimizing the computation of coordinate matrices._wigner_discretized_iterative
function by precomputing exponential terms and square roots, reducing redundant calculations, and reusing matrices to enhance memory efficiency.Changes walkthrough 📝
tensors.py
Optimize tensor network wire and shape handling
mrmustard/math/tensor_networks/tensors.py
Wire
class attributes usingfield
.comprehensions.
AdjointView
andDualView
initializations.wigner.py
Enhance Wigner function computation efficiency
mrmustard/physics/wigner.py
make_grid
function with preallocated arrays._wigner_discretized_iterative
.