Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symplectic override on Ggate #495

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Symplectic override on Ggate #495

wants to merge 3 commits into from

Conversation

apchytr
Copy link
Collaborator

@apchytr apchytr commented Sep 26, 2024

User description

Context: Ggate.symplectic should probably be returning the symplectic matrix it's defined with rather than recomputing.

Description of the Change: Overrride symplectic on Ggate to return the symplectic matrix stored in the param set.

Benefits: Avoid having to recompute the symplectic matrix


PR Type

enhancement, tests


Description

  • Enhanced the Ggate class by overriding the symplectic property to return the symplectic matrix stored in the parameter set, avoiding recomputation.
  • Updated the initialization method to use _add_parameter for adding the symplectic parameter.
  • Added a test case to verify that the symplectic property returns the correct symplectic matrix.

Changes walkthrough 📝

Relevant files
Enhancement
ggate.py
Override `symplectic` property to return stored matrix     

mrmustard/lab_dev/transformations/ggate.py

  • Added a property symplectic to return the stored symplectic matrix.
  • Replaced parameter_set.add_parameter with _add_parameter.
  • +7/-3     
    Tests
    test_ggate.py
    Add test for `symplectic` property in `Ggate`                       

    tests/test_lab_dev/test_transformations/test_ggate.py

  • Added an assertion to check if symplectic returns the correct matrix.
  • +1/-0     

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

    @apchytr apchytr added the no changelog Pull request does not require a CHANGELOG entry label Sep 26, 2024
    @apchytr apchytr requested a review from ziofil September 26, 2024 18:41
    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added enhancement New feature or request tests labels Sep 26, 2024
    Copy link

    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: 95
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    Copy link

    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
    Best practice
    Add a tolerance parameter to the assertion for more robust testing

    Consider adding a tolerance parameter to the math.allclose() function call. This can
    help prevent potential issues with floating-point precision and make the test more
    robust.

    tests/test_lab_dev/test_transformations/test_ggate.py [33]

    -assert math.allclose(Eye.symplectic, math.eye(2))
    +assert math.allclose(Eye.symplectic, math.eye(2), atol=1e-8)
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Introducing a tolerance parameter to the 'math.allclose()' function call increases the robustness of the test by accounting for floating-point precision issues. This is an important enhancement for ensuring reliable test results.

    9
    Add a docstring to the property for improved documentation

    Consider adding a docstring to the symplectic property to explain its purpose and
    return value. This will improve code documentation and make it easier for other
    developers to understand and use the property.

    mrmustard/lab_dev/transformations/ggate.py [66-68]

     @property
     def symplectic(self):
    +    """
    +    Returns the symplectic matrix associated with this Ggate.
    +
    +    Returns:
    +        RealMatrix: The symplectic matrix.
    +    """
         return self.parameter_set.symplectic.value
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a docstring to the 'symplectic' property improves code documentation, making it easier for other developers to understand the purpose and return value of the property. This is a best practice that enhances code maintainability and usability.

    8
    Enhancement
    Improve parameter naming for better code readability

    Consider using a more descriptive name for the parameter. Instead of s, use
    symplectic_matrix to improve code readability and make the purpose of the parameter
    clearer.

    mrmustard/lab_dev/transformations/ggate.py [61-64]

     self._representation = Bargmann.from_function(
    -    fn=lambda s: Unitary.from_symplectic(modes, s).bargmann_triple(),
    +    fn=lambda symplectic_matrix: Unitary.from_symplectic(modes, symplectic_matrix).bargmann_triple(),
         s=self.parameter_set.symplectic,
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to rename the parameter from 's' to 'symplectic_matrix' enhances code readability and clarity, making the code easier to understand and maintain. This is a valuable improvement, although it does not affect the functionality.

    7

    💡 Need additional feedback ? start a PR chat

    Copy link

    codecov bot commented Sep 27, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 89.74%. Comparing base (068bdf7) to head (23f8548).

    Additional details and impacted files
    @@           Coverage Diff            @@
    ##           develop     #495   +/-   ##
    ========================================
      Coverage    89.73%   89.74%           
    ========================================
      Files          104      104           
      Lines         7621     7623    +2     
    ========================================
    + Hits          6839     6841    +2     
      Misses         782      782           
    Files with missing lines Coverage Δ
    mrmustard/lab_dev/transformations/ggate.py 100.00% <100.00%> (ø)

    Continue to review full report in Codecov by Sentry.

    Legend - Click here to learn more
    Δ = absolute <relative> (impact), ø = not affected, ? = missing data
    Powered by Codecov. Last update 068bdf7...23f8548. Read the comment docs.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    enhancement New feature or request no changelog Pull request does not require a CHANGELOG entry Review effort [1-5]: 2 tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant