search

XanaduAI / MrMustard

A differentiable bridge between phase space and Fock space
https://mrmustard.readthedocs.io/
Apache License 2.0
78 stars 27 forks source link

Avoid storing original_abc_data when c is not a scalar #493

Closed apchytr closed 1 month ago

apchytr commented 1 month ago

User description

Context: After to_bargmann() original bargmann data bug we always store the original Bargmann data when calling to_fock. This isn't ideal in the case when c is an array where instead keeping the array is more efficient.

Description of the Change: to_fock checks the polynomial shape and only sets ._original_abc_data if c is a scalar

PR Type

Bug fix, Tests

Description

Changes walkthrough πŸ“

Relevant files
Bug fix
circuit_components.py
Conditional storage of `_original_abc_data` based on polynomial shape
mrmustard/lab_dev/circuit_components.py
  • Added a condition to check if polynomial_shape[0] is zero before
    setting _original_abc_data.
  • Ensured _original_abc_data is only set when the condition is met.
    		•  +2/-1     
    Tests
    test_circuit_components.py
    Add tests for `_original_abc_data` conditional logic         
    tests/test_lab_dev/test_circuit_components.py
  • Added assertions to verify the correct setting of _original_abc_data.
  • Updated tests to check for None when c is not a scalar.
    		•  +4/-2     

    πŸ’‘ 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 πŸ”

    ⏱️ Estimated effort to review: 2 πŸ”΅πŸ”΅βšͺβšͺβšͺ
    πŸ… Score: 92
    πŸ§ͺ PR contains tests
    πŸ”’ No security concerns identified
    ⚑ Key issues to review

    Potential Bug
    The condition for setting _original_abc_data might be too restrictive. It only checks if polynomial_shape[0] is 0, which may not cover all cases where c is a scalar.
    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 ✨

    CategorySuggestion                                                                                                                                    Score
    Test coverage
    Add a test case for scalar 'c' to ensure correct behavior ___ **Consider adding a test case where c is a scalar to ensure the _original_abc_data is
    set correctly in that scenario.** [tests/test_lab_dev/test_circuit_components.py [204-209]](https://github.com/XanaduAI/MrMustard/pull/493/files#diff-cf594d34002781ecbd6f00d55be035f5d6821af5cfde94038830e0e3ea5f5bc3R204-R209) ```diff -c = np.random.random((1, 5)) -barg = Bargmann(A, b, c) -fock_cc = CircuitComponent(barg, wires=[(), (), (0, 1), ()]).to_fock(shape=(10, 10)) +# Test with non-scalar c +c_non_scalar = np.random.random((1, 5)) +barg_non_scalar = Bargmann(A, b, c_non_scalar) +fock_cc_non_scalar = CircuitComponent(barg_non_scalar, wires=[(), (), (0, 1), ()]).to_fock(shape=(10, 10)) poly = math.hermite_renormalized(A, b, 1, (10, 10, 5)) -assert fock_cc.representation.ansatz._original_abc_data is None -assert np.allclose(fock_cc.representation.data, np.einsum("ijk,k", poly, c[0])) +assert fock_cc_non_scalar.representation.ansatz._original_abc_data is None +assert np.allclose(fock_cc_non_scalar.representation.data, np.einsum("ijk,k", poly, c_non_scalar[0])) +# Test with scalar c +c_scalar = np.random.random() +barg_scalar = Bargmann(A, b, c_scalar) +fock_cc_scalar = CircuitComponent(barg_scalar, wires=[(), (), (0, 1), ()]).to_fock(shape=(10, 10)) +assert fock_cc_scalar.representation.ansatz._original_abc_data == barg_scalar.triple + ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: Adding a test case for scalar `c` enhances test coverage and ensures that the new condition behaves correctly in all scenarios, which is crucial for maintaining code reliability.
    		9
    Readability
    Use a more explicit check to determine if 'c' is a scalar ___ **Consider using a more descriptive condition to check if c is a scalar. The current
    check self.representation.ansatz.polynomial_shape[0] == 0 might not be the most
    intuitive way to determine if c is a scalar.** [mrmustard/lab_dev/circuit_components.py [575-576]](https://github.com/XanaduAI/MrMustard/pull/493/files#diff-05cc8f1f470b3eab23b8a8b1b1a628a97c87852722ea6b5408e1bb70efd4ab72R575-R576) ```diff -if self.representation.ansatz.polynomial_shape[0] == 0: +if np.isscalar(self.representation.ansatz.c): fock.ansatz._original_abc_data = self.representation.triple ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: The suggestion improves code readability by using a more explicit check for scalar values, which can make the code easier to understand and maintain. However, it assumes that `c` is directly accessible and relevant in this context, which may not be the case.
    		7

    πŸ’‘ 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.66%. Comparing base (69a9a2b) to head (760dd6b). Report is 1 commits behind head on develop.

    Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #493 +/- ## ======================================== Coverage 89.65% 89.66% ======================================== Files 103 103 Lines 7456 7457 +1 ======================================== + Hits 6685 6686 +1 Misses 771 771 ``` | [Files with missing lines](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/493?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI) | Coverage Ξ” | | |---|---|---| | [mrmustard/lab\_dev/circuit\_components.py](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/493?src=pr&el=tree&filepath=mrmustard%2Flab_dev%2Fcircuit_components.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI#diff-bXJtdXN0YXJkL2xhYl9kZXYvY2lyY3VpdF9jb21wb25lbnRzLnB5) | `99.08% <100.00%> (+<0.01%)` | :arrow_up: | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/493?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/493?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI). Last update [69a9a2b...760dd6b](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/493?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).