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

Bugfix: c broadcasting in gaussian integral #508

Closed ziofil closed 1 month ago

ziofil commented 1 month ago

User description

Fixes bug by reshaping some parts of a product correctly

PR Type

Bug fix, Tests

Description

Changes walkthrough 📝

Relevant files
Bug fix
gaussian_integrals.py
Fix broadcasting issue in Gaussian integral calculation   
mrmustard/physics/gaussian_integrals.py
  • Fixed broadcasting issue in the calculation of c_post.
  • Reshaped the result to match the shape of c.
    		•  +4/-4     
    Tests
    test_gaussian_integrals.py
    Add test for batched Gaussian integral with polynomial input
    tests/test_physics/test_gaussian_integrals.py
  • Added a new test for batched inputs with polynomial c.
  • Verified the shape of the result matches expected dimensions.
    		•  +12/-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: 2 🔵🔵⚪⚪⚪
    🏅 Score: 95
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Performance Impact
    The reshaping operation might have a slight performance impact. Verify if this is necessary for all cases or if there's a more efficient way to handle the broadcasting.
    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:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Enhance test coverage by verifying the correctness of the function's output values ___ **Add assertions to check the actual values of the result, not just the shapes, to
    ensure the function is producing correct output.** [tests/test_physics/test_gaussian_integrals.py [308-311]](https://github.com/XanaduAI/MrMustard/pull/508/files#diff-f64960aa126fc9053f056b46e568e9a1cd22ccb908dbccad294f7d67a1fbbb1bR308-R311) ```diff -res = complex_gaussian_integral_1((A, b, c), [0], [1]) # pylint: disable=pointless-statement +res = complex_gaussian_integral_1((A, b, c), [0], [1]) assert res[0].shape == (4, 2, 2) assert res[1].shape == (4, 2) assert res[2].shape == (4, 2, 2) +assert np.allclose(res[0], expected_A_result) +assert np.allclose(res[1], expected_b_result) +assert np.allclose(res[2], expected_c_result) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Adding assertions to check the actual values of the result, not just the shapes, significantly improves test coverage and ensures the function produces correct outputs. This enhancement is important for verifying the function's correctness.
    		8
    Replace reshape operation with broadcast for better compatibility and potential performance improvement ___ **Consider using math.broadcast_to instead of math.reshape to ensure compatibility
    with different tensor libraries and potentially improve performance.** [mrmustard/physics/gaussian_integrals.py [383-387]](https://github.com/XanaduAI/MrMustard/pull/508/files#diff-a7b779ea109bae98fb3be0cb7ed5f6dba566d967f72666d24fbd01015f7962dcR383-R387) ```diff -c_post = c * math.reshape( +c_post = c * math.broadcast_to( math.sqrt(math.cast((-1) ** m / det_M, "complex128")) * math.exp(-0.5 * math.sum(bM * math.solve(M, bM), axes=[-1])), - c.shape[:1] + (1,) * (len(c.shape) - 1), + c.shape ) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Using `math.broadcast_to` instead of `math.reshape` can improve compatibility with different tensor libraries and potentially enhance performance. This suggestion is relevant and could lead to more robust code, but it does not address a critical issue.
    		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.77%. Comparing base (a24b0bf) to head (a3c0bb3). Report is 1 commits behind head on develop.

    Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #508 +/- ## ======================================== Coverage 89.77% 89.77% ======================================== Files 104 104 Lines 7639 7639 ======================================== Hits 6858 6858 Misses 781 781 ``` | [Files with missing lines](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/508?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/gaussian\_integrals.py](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/508?src=pr&el=tree&filepath=mrmustard%2Fphysics%2Fgaussian_integrals.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI#diff-bXJtdXN0YXJkL3BoeXNpY3MvZ2F1c3NpYW5faW50ZWdyYWxzLnB5) | `99.40% <100.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/508?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/508?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI). Last update [a24b0bf...a3c0bb3](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/508?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).