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

separated ket dm from base #510

Merged
merged 14 commits into from
Oct 23, 2024
Merged

separated ket dm from base #510

merged 14 commits into from
Oct 23, 2024

Conversation

ziofil
Copy link
Collaborator

@ziofil ziofil commented Oct 17, 2024

User description

Context:
The file mrmustard/lab_dev/states/base.py was 1200+ lines long and it was hard to navigate.

Description of the Change:
DM and Ket are now defined in their own files.

Benefits:

  1. Easier to navigate and understand where we are in the file (e.g. sometimes one had to scroll up or down to check whether a method with the same name was part of Ket or DM)
  2. Removed a few ugly instance checks
  3. states/base.py now is more abstract

Possible Drawbacks:
Some minor code duplication because e.g. DM cannot use Ket.random in its own random method anymore (unless we allow for imports in methods).


PR Type

enhancement


Description

  • Separated Ket and DM classes from base.py into their own modules, ket.py and dm.py.
  • Updated all relevant state files to import Ket and DM from their new modules.
  • Refactored import statements in __init__.py for clarity and specificity.
  • Adjusted test cases to reflect the new module structure.

Changes walkthrough 📝

Relevant files
Enhancement
13 files
__init__.py
Refactor import statements for clarity and specificity     

mrmustard/lab_dev/states/init.py

  • Updated import statements to import specific classes instead of using
    wildcard imports.
  • Added explicit imports for Ket and DM classes.
  • +12/-10 
    base.py
    Refactor base state class to remove Ket and DM                     

    mrmustard/lab_dev/states/base.py

  • Removed Ket and DM class definitions from the file.
  • Adjusted imports and references to use new ket and dm modules.
  • Simplified the from_phase_space method return type.
  • +7/-700 
    coherent.py
    Update import for Ket class in coherent state                       

    mrmustard/lab_dev/states/coherent.py

    • Updated import to use Ket from the new ket module.
    +1/-1     
    displaced_squeezed.py
    Update import for Ket class in displaced squeezed state   

    mrmustard/lab_dev/states/displaced_squeezed.py

    • Updated import to use Ket from the new ket module.
    +1/-1     
    dm.py
    Create DM class in a separate module                                         

    mrmustard/lab_dev/states/dm.py

  • Created a new file for the DM class.
  • Implemented methods for density matrix operations and properties.
  • +421/-0 
    ket.py
    Create Ket class in a separate module                                       

    mrmustard/lab_dev/states/ket.py

  • Created a new file for the Ket class.
  • Implemented methods for ket state operations and properties.
  • +362/-0 
    number.py
    Update import for Ket class in number state                           

    mrmustard/lab_dev/states/number.py

    • Updated import to use Ket from the new ket module.
    +1/-1     
    quadrature_eigenstate.py
    Update import for Ket class in quadrature eigenstate         

    mrmustard/lab_dev/states/quadrature_eigenstate.py

    • Updated import to use Ket from the new ket module.
    +1/-1     
    sauron.py
    Update import for Ket class in Sauron state                           

    mrmustard/lab_dev/states/sauron.py

    • Updated import to use Ket from the new ket module.
    +1/-1     
    squeezed_vacuum.py
    Update import for Ket class in squeezed vacuum state         

    mrmustard/lab_dev/states/squeezed_vacuum.py

    • Updated import to use Ket from the new ket module.
    +1/-1     
    thermal.py
    Update import for DM class in thermal state                           

    mrmustard/lab_dev/states/thermal.py

    • Updated import to use DM from the new dm module.
    +1/-1     
    two_mode_squeezed_vacuum.py
    Update import for Ket class in two-mode squeezed vacuum state

    mrmustard/lab_dev/states/two_mode_squeezed_vacuum.py

    • Updated import to use Ket from the new ket module.
    +1/-1     
    vacuum.py
    Update import for Ket class in vacuum state                           

    mrmustard/lab_dev/states/vacuum.py

    • Updated import to use Ket from the new ket module.
    +1/-1     
    Tests
    1 files
    test_ansatz.py
    Update import for DM class in test cases                                 

    tests/test_physics/test_ansatz.py

    • Updated import to use DM from the new dm module.
    +1/-1     

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

    Copy link
    Contributor

    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: 4 🔵🔵🔵🔵⚪
    🏅 Score: 85
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication
    The auto_shape method is duplicated in both Ket and DM classes with very similar implementation. Consider refactoring to reduce duplication.

    Performance Concern
    The random method creates a full symplectic matrix and then extracts submatrices, which might be inefficient for large mode numbers. Consider optimizing this process.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Oct 17, 2024

    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
    ✅ Use a more numerically stable method for matrix inversion
    Suggestion Impact:The suggestion to use math.solve instead of math.inv was implemented, improving numerical stability. However, the implementation used math.transpose and math.dagger, which differs slightly from the suggestion.

    code diff:

    -        A = S_2 @ math.conj(math.inv(S_1))  # use solve for inverse
    +        A = math.transpose(math.solve(math.dagger(S_1), math.transpose(S_2)))

    Consider using math.solve instead of math.inv for better numerical stability when
    computing the inverse of a matrix.

    mrmustard/lab_dev/states/ket.py [210]

    -A = S_2 @ math.conj(math.inv(S_1))  # use solve for inverse
    +A = math.solve(math.conj(S_1), S_2.T).T
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion to use math.solve instead of math.inv is a best practice for improving numerical stability when computing matrix inverses. This change enhances the robustness of the code.

    9
    ✅ Group related imports together to improve code organization and readability

    Consider grouping related imports together, such as placing Ket and DM imports next
    to each other, to improve code organization and readability.

    mrmustard/lab_dev/states/init.py [19-30]

     from .base import State
     from .ket import Ket
     from .dm import DM
    +
     from .coherent import Coherent
     from .displaced_squeezed import DisplacedSqueezed
     from .number import Number
     from .quadrature_eigenstate import QuadratureEigenstate
     from .squeezed_vacuum import SqueezedVacuum
     from .thermal import Thermal
     from .two_mode_squeezed_vacuum import TwoModeSqueezedVacuum
     from .vacuum import Vacuum
     from .sauron import Sauron

    [Suggestion has been applied]

    Suggestion importance[1-10]: 5

    Why: Grouping related imports can enhance readability and organization, but the impact is minor since the current order is already clear. The suggestion is valid but offers only a slight improvement.

    5
    Performance
    ✅ Use a more efficient method to check if a matrix is Hermitian

    Consider using a more efficient method to check if gamma_A is Hermitian, such as
    np.allclose(gamma_A, gamma_A.conj().T) instead of comparing norms.

    mrmustard/lab_dev/states/dm.py [79-82]

    -if (
    -    math.real(math.norm(gamma_A - math.conj(gamma_A.T))) > settings.ATOL
    -):  # checks if gamma_A is Hermitian
    +if not math.allclose(gamma_A, math.conj(gamma_A.T), atol=settings.ATOL):  # checks if gamma_A is Hermitian
         return False

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: The suggestion to use math.allclose instead of comparing norms is valid and improves the efficiency and readability of the code. It directly checks if the matrix is Hermitian, which aligns with the intended functionality.

    8

    💡 Need additional feedback ? start a PR chat

    @ziofil ziofil added the no changelog Pull request does not require a CHANGELOG entry label Oct 17, 2024
    ziofil and others added 2 commits October 17, 2024 10:34
    Co-authored-by: codiumai-pr-agent-pro[bot] <151058649+codiumai-pr-agent-pro[bot]@users.noreply.github.com>
    ziofil and others added 4 commits October 17, 2024 10:38
    Co-authored-by: codiumai-pr-agent-pro[bot] <151058649+codiumai-pr-agent-pro[bot]@users.noreply.github.com>
    Copy link

    codecov bot commented Oct 17, 2024

    Codecov Report

    Attention: Patch coverage is 97.23926% with 9 lines in your changes missing coverage. Please review.

    Project coverage is 89.41%. Comparing base (6ab5aec) to head (2805e5f).
    Report is 1 commits behind head on develop.

    Files with missing lines Patch % Lines
    mrmustard/lab_dev/states/dm.py 95.56% 7 Missing ⚠️
    mrmustard/lab_dev/states/ket.py 98.57% 2 Missing ⚠️
    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #510      +/-   ##
    ===========================================
    + Coverage    89.33%   89.41%   +0.07%     
    ===========================================
      Files           87       89       +2     
      Lines         5984     6029      +45     
    ===========================================
    + Hits          5346     5391      +45     
      Misses         638      638              
    Files with missing lines Coverage Δ
    mrmustard/lab_dev/states/__init__.py 100.00% <100.00%> (ø)
    mrmustard/lab_dev/states/base.py 96.57% <100.00%> (+0.06%) ⬆️
    mrmustard/lab_dev/states/coherent.py 100.00% <100.00%> (ø)
    mrmustard/lab_dev/states/displaced_squeezed.py 100.00% <100.00%> (ø)
    mrmustard/lab_dev/states/number.py 100.00% <100.00%> (ø)
    mrmustard/lab_dev/states/quadrature_eigenstate.py 100.00% <100.00%> (ø)
    mrmustard/lab_dev/states/sauron.py 100.00% <100.00%> (ø)
    mrmustard/lab_dev/states/squeezed_vacuum.py 100.00% <100.00%> (ø)
    mrmustard/lab_dev/states/thermal.py 100.00% <100.00%> (ø)
    ...mustard/lab_dev/states/two_mode_squeezed_vacuum.py 100.00% <100.00%> (ø)
    ... and 3 more

    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 6ab5aec...2805e5f. Read the comment docs.

    @ziofil ziofil requested a review from apchytr October 17, 2024 18:58
    Copy link
    Collaborator

    @apchytr apchytr left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Some minor comments but looks great thanks!

    @ziofil ziofil merged commit 67b8624 into develop Oct 23, 2024
    8 checks passed
    @ziofil ziofil deleted the separate_ket_dm_from_base branch October 23, 2024 20:24
    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]: 4
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants