adds padding when num poly wires is different

ziofil commented 1 month ago

User description

We allow making linear superpositions of components with different number of polynomial wires by padding A and b with zeros, and adding empty dimensions to the c array.

PR Type

Enhancement, Tests

Description

Enhanced the addition operation in ansatze.py to pad matrices A and b when the number of polynomial wires differs, ensuring compatibility.
Introduced a check to raise a ValueError if polynomial shapes are inconsistent.
Expanded dimensions of arrays to handle different shapes during addition.
Added tests to verify correct padding of A and b and to ensure a ValueError is raised for inconsistent polynomial shapes.

Changes walkthrough 📝

Relevant files

Enhancement

ansatze.py

Add padding for polynomial wires in ansatze addition

mrmustard/physics/ansatze.py

Added padding for A and b matrices when polynomial wires differ.

Implemented checks for consistent polynomial shapes.

Expanded dimensions of arrays to accommodate different shapes.

+18/-1

Tests

test_ansatz.py

Add tests for polynomial wire padding and shape consistency

tests/test_physics/test_ansatz.py

Added test for padding A and b with different polynomial wires.

Added test for inconsistent polynomial shapes raising a ValueError.

+31/-0

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

codiumai-pr-agent-pro[bot] commented 1 month ago

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🏅 Score: 85

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Potential Performance Issue
The repeated use of math.expand_dims in a loop might be inefficient for large differences in polynomial wire counts. Error Handling
The error message for inconsistent polynomial shapes could be more informative by including the actual shapes.

codiumai-pr-agent-pro[bot] commented 1 month ago

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Enhancement	Parameterize test cases to increase coverage and reduce duplication ___ Consider parameterizing the test cases for different polynomial wire configurations to increase test coverage and reduce code duplication. This can be achieved using pytest's parameterize decorator. [tests/test_physics/test_ansatz.py [614-631]](https://github.com/XanaduAI/MrMustard/pull/499/files#diff-8ae3f2522d16e5dd99914d95d64920f61fb075eb99cec87ee2a122872331da59R614-R631) ```diff -def test_add_different_poly_wires(self): +@pytest.mark.parametrize("shape1,shape2,expected", [ + ((2, 2), (3, 3), (3, 3)), + ((3, 3), (2, 2), (3, 3)), +]) +def test_add_different_poly_wires(self, shape1, shape2, expected): "tests that A and b are padded correctly" - A1 = np.random.random((1, 2, 2)) - A2 = np.random.random((1, 3, 3)) - b1 = np.random.random((1, 2)) - b2 = np.random.random((1, 3)) + A1 = np.random.random((1,) + shape1) + A2 = np.random.random((1,) + shape2) + b1 = np.random.random((1,) + shape1[:1]) + b2 = np.random.random((1,) + shape2[:1]) c1 = np.random.random((1,)) c2 = np.random.random((1, 11)) ansatz1 = PolyExpAnsatz(A1, b1, c1) ansatz2 = PolyExpAnsatz(A2, b2, c2) ansatz_sum = ansatz1 + ansatz2 - assert ansatz_sum.mat.shape == (2, 3, 3) - assert ansatz_sum.vec.shape == (2, 3) - assert ansatz_sum.array.shape == (2, 11) - ansatz_sum = ansatz2 + ansatz1 - assert ansatz_sum.mat.shape == (2, 3, 3) - assert ansatz_sum.vec.shape == (2, 3) + assert ansatz_sum.mat.shape == (2,) + expected + assert ansatz_sum.vec.shape == (2,) + expected[:1] assert ansatz_sum.array.shape == (2, 11) ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: Parameterizing test cases increases test coverage and reduces code duplication, which enhances the robustness and maintainability of the test suite. This is a valuable improvement for ensuring code quality.	8
Enhancement	✅ Extract padding logic into a separate method for better code organization ___ Suggestion Impact: The commit extracted the padding logic into a separate method, similar to the suggestion, improving code organization and reusability. code diff: ```diff + def pad_and_expand(mat, vec, array, target_size): + pad_size = target_size - mat.shape[-1] + padded_mat = math.pad(mat, ((0, 0), (0, pad_size), (0, pad_size))) + padded_vec = math.pad(vec, ((0, 0), (0, pad_size))) + padding_array = math.ones((1,) * pad_size, dtype=array.dtype) + expanded_array = math.outer(array, padding_array) + return padded_mat, padded_vec, expanded_array ``` ___ Consider extracting the padding logic into a separate method to improve code organization and reusability. This would make the __add__ method cleaner and easier to understand. [mrmustard/physics/ansatze.py [429-438]](https://github.com/XanaduAI/MrMustard/pull/499/files#diff-85405583f9e1c800d7cbad37cca5f7da9c0181a18dd40db2a207dcd134928fe0R429-R438) ```diff -if n < m: - self.mat = math.pad(self.mat, ((0, 0), (0, m - n), (0, m - n))) - self.vec = math.pad(self.vec, ((0, 0), (0, m - n))) - for _ in range(m - n): - self.array = math.expand_dims(self.array, axis=-1) -elif n > m: - other.mat = math.pad(other.mat, ((0, 0), (0, n - m), (0, n - m))) - other.vec = math.pad(other.vec, ((0, 0), (0, n - m))) - for _ in range(n - m): - other.array = math.expand_dims(other.array, axis=-1) +def _pad_ansatz(self, other, diff): + if diff > 0: + other.mat = math.pad(other.mat, ((0, 0), (0, diff), (0, diff))) + other.vec = math.pad(other.vec, ((0, 0), (0, diff))) + for _ in range(diff): + other.array = math.expand_dims(other.array, axis=-1) + else: + self.mat = math.pad(self.mat, ((0, 0), (0, -diff), (0, -diff))) + self.vec = math.pad(self.vec, ((0, 0), (0, -diff))) + for _ in range(-diff): + self.array = math.expand_dims(self.array, axis=-1) +diff = n - m +if diff != 0: + self._pad_ansatz(other, diff) + ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: Extracting the padding logic into a separate method improves code organization and reusability, making the `__add__` method cleaner and easier to understand. This enhances maintainability without affecting functionality.	7
Performance	✅ Use more efficient array operations instead of loops for dimension expansion ___ Suggestion Impact: The commit replaced the for loop used for expanding the dimensions of the array with a more efficient operation using math.outer, which aligns with the suggestion to use more efficient array operations. code diff: ```diff + def pad_and_expand(mat, vec, array, target_size): + pad_size = target_size - mat.shape[-1] + padded_mat = math.pad(mat, ((0, 0), (0, pad_size), (0, pad_size))) + padded_vec = math.pad(vec, ((0, 0), (0, pad_size))) + padding_array = math.ones((1,) * pad_size, dtype=array.dtype) + expanded_array = math.outer(array, padding_array) + return padded_mat, padded_vec, expanded_array ``` ___ Consider using a list comprehension or numpy's repeat function instead of a for loop to expand the dimensions of the array. This could potentially improve performance and make the code more concise. [mrmustard/physics/ansatze.py [432-433]](https://github.com/XanaduAI/MrMustard/pull/499/files#diff-85405583f9e1c800d7cbad37cca5f7da9c0181a18dd40db2a207dcd134928fe0R432-R433) ```diff -for _ in range(m - n): - self.array = math.expand_dims(self.array, axis=-1) +self.array = math.repeat(math.expand_dims(self.array, axis=-1), m - n, axis=-1) ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: Using more efficient array operations can improve performance and make the code more concise. However, the performance gain might be marginal depending on the context, so the impact is moderate.	6
Maintainability	✅ Use more descriptive variable names to enhance code readability ___ Suggestion Impact: The suggestion to use more descriptive variable names was partially implemented. The variables 'n' and 'm' were renamed to 'n1' and 'n2', which are more descriptive than the original names, though not exactly as suggested. code diff: ```diff - (_, n, _) = self.mat.shape - (_, m, _) = other.mat.shape + (_, n1, _) = self.mat.shape + (_, n2, _) = other.mat.shape ``` ___ Consider using a more descriptive variable name instead of 'n' and 'm' to improve code readability. For example, 'self_num_wires' and 'other_num_wires' would be more explicit about what these variables represent. [mrmustard/physics/ansatze.py [423-424]](https://github.com/XanaduAI/MrMustard/pull/499/files#diff-85405583f9e1c800d7cbad37cca5f7da9c0181a18dd40db2a207dcd134928fe0R423-R424) ```diff -(_, n, _) = self.mat.shape -(_, m, _) = other.mat.shape +(_, self_num_wires, _) = self.mat.shape +(_, other_num_wires, _) = other.mat.shape ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 5 Why: The suggestion to use more descriptive variable names improves code readability and maintainability, making it clearer what the variables represent. However, it does not address any functional issues.	5

💡 Need additional feedback ? start a PR chat

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.76%. Comparing base (a75c1c2) to head (f5f5dad). Report is 1 commits behind head on develop.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #499 +/- ## =========================================== + Coverage 89.74% 89.76% +0.02% =========================================== Files 104 104 Lines 7623 7639 +16 =========================================== + Hits 6841 6857 +16 Misses 782 782 ``` | [Files with missing lines](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/499?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI) | Coverage Δ | | |---|---|---| | [mrmustard/physics/ansatze.py](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/499?src=pr&el=tree&filepath=mrmustard%2Fphysics%2Fansatze.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI#diff-bXJtdXN0YXJkL3BoeXNpY3MvYW5zYXR6ZS5weQ==) | `98.02% <100.00%> (+0.07%)` | :arrow_up: | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/499?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/499?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI). Last update [a75c1c2...f5f5dad](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/499?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI).

XanaduAI / MrMustard