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

BtoF #522

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

BtoF #522

wants to merge 12 commits into from

Conversation

ziofil
Copy link
Collaborator

@ziofil ziofil commented Nov 12, 2024

User description

BtoF - will update description


PR Type

enhancement, bug_fix, tests


Description

  • Refactored the PolyExpAnsatz class to improve its functionality, including new methods for ansatz generation, decomposition, and evaluation.
  • Replaced polynomial_shape with num_DV_vars across multiple files for consistency and correctness.
  • Renamed reorder_abc to reorder_Abc and updated related tests and functions.
  • Fixed formatting issues in configuration and training files.
  • Enhanced tests to align with the updated class and function implementations.

Changes walkthrough 📝

Relevant files
Configuration changes
1 files
conf.py
Update configuration for mathjax and theme options             

doc/conf.py

  • Updated mathjax_path formatting for consistency.
  • Fixed string concatenation in html_theme_options.
  • +4/-3     
    Bug fix
    4 files
    dm.py
    Update condition check to use `num_DV_vars`                           

    mrmustard/lab_dev/states/dm.py

  • Replaced polynomial_shape with num_DV_vars for condition check.
  • +1/-1     
    ket.py
    Update condition check to use `num_DV_vars`                           

    mrmustard/lab_dev/states/ket.py

  • Replaced polynomial_shape with num_DV_vars for condition check.
  • +1/-1     
    representations.py
    Update condition checks in representations                             

    mrmustard/physics/representations.py

  • Replaced polynomial_shape with num_DV_vars for condition check.
  • +2/-2     
    trainer.py
    Correct formatting in `ray.wait` call                                       

    mrmustard/training/trainer.py

    • Fixed indentation and formatting in ray.wait call.
    +2/-2     
    Enhancement
    2 files
    polyexp_ansatz.py
    Refactor and enhance `PolyExpAnsatz` class functionality 

    mrmustard/physics/ansatz/polyexp_ansatz.py

  • Refactored PolyExpAnsatz class with new methods and properties.
  • Added handling for num_derived_vars and num_DV_vars.
  • Improved ansatz decomposition and simplification logic.
  • Enhanced evaluation and partial evaluation methods.
  • +392/-422
    gaussian_integrals.py
    Refactor and rename functions for consistency                       

    mrmustard/physics/gaussian_integrals.py

  • Renamed reorder_abc to reorder_Abc.
  • Updated join_Abc method to use math.concat.
  • +14/-14 
    Tests
    2 files
    test_polyexp_ansatz.py
    Update tests for `PolyExpAnsatz` with new properties         

    tests/test_physics/test_ansatz/test_polyexp_ansatz.py

    • Updated tests to use num_DV_vars instead of polynomial_shape.
    +4/-6     
    test_gaussian_integrals.py
    Update tests for renamed function `reorder_Abc`                   

    tests/test_physics/test_gaussian_integrals.py

    • Updated tests to use reorder_Abc instead of reorder_abc.
    +7/-7     

    💡 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
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Bug
    The _partial_eval_single method is called with incorrect number of arguments in _partial_eval method. The indices parameter is missing when calling _partial_eval_single.

    Performance Issue
    Multiple concatenations in join_Abc could be combined into a single operation to improve performance

    mrmustard/physics/ansatz/polyexp_ansatz.py Show resolved Hide resolved
    )
    )
    for Ai, bi, ci in zip(self.A, self.b, self.c):
    Abc.append(self._partial_eval_single(Ai, bi, ci, z))
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    The _partial_eval method should pass the indices parameter when calling _partial_eval_single. Change line 531 to: Abc.append(self._partial_eval_single(Ai, bi, ci, z, indices)) [important]

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Nov 12, 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
    Replace string concatenation with a single string literal to avoid potential whitespace issues

    The string concatenation using "+" operator for the copyright text is error-prone
    and could lead to unexpected whitespace. Use a single string literal instead.

    doc/conf.py [144]

    -"TensorFlow, the TensorFlow logo, and any related marks are trademarks " "of Google Inc."
    +"TensorFlow, the TensorFlow logo, and any related marks are trademarks of Google Inc."
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion correctly identifies a potential issue with string concatenation that could lead to unwanted whitespace. While valid, the impact is minor as the current code still works correctly.

    5
    Possible issue
    Add shape validation before array concatenation

    Add validation to ensure input arrays have compatible shapes before concatenation.

    mrmustard/physics/gaussian_integrals.py [265-271]

    +if A1Z.shape[-1] != ZA2.shape[-1]:
    +    raise ValueError(f"Incompatible shapes for concatenation: {A1Z.shape}, {ZA2.shape}")
     A = math.concat([A1Z, ZA2], axis=-2)
     A = math.concat(
         [
             A[:, :n1, :],
             A[:, nA1 : nA1 + n2, :],
         ],
         axis=-2,
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: Adding shape validation before concatenation could help catch potential errors earlier with clearer error messages, though the math.concat operation would raise an error anyway if shapes are incompatible.

    4
    Maintainability
    Place inline code comment at the correct line to properly disable the intended lint warning

    Move the pylint disable comment to the line where it's actually needed, as it's
    currently misplaced and disabling the wrong line.

    mrmustard/training/trainer.py [397-399]

    -results, running_tasks = ray.wait(
    +results, running_tasks = ray.wait(  # pylint: disable=unused-variable
         promises, num_returns=len(promises)
    -)  # pylint: disable=unused-variable
    +)
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: The suggestion correctly points out that the pylint disable comment should be moved to the line where the unused variable is declared. While this improves code clarity, it's a minor maintainability enhancement.

    4
    Performance
    Cache computed values to avoid redundant calculations

    Cache the computed exponential part to avoid recalculating it when combining with
    polynomial part.

    mrmustard/physics/ansatz/polyexp_ansatz.py [466-472]

     exp_sum = self._compute_exp_part(z)  # shape (batch_size, k)
     if self.num_derived_vars == 0:
         ret = math.einsum("ik,i...->k...", exp_sum, self.c)
     else:
         poly = self._compute_polynomial_part(z)  # shape (batch_size, k, *derived_shape)
         ret = self._combine_exp_and_poly(exp_sum, poly)
    +self._cached_exp_sum = exp_sum  # Cache for potential reuse
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While caching could improve performance in some cases, the benefit is likely minimal as the cached value would only be useful if the same z input is used multiple times, which is not a common use case.

    3

    💡 Need additional feedback ? start a PR chat

    @ziofil ziofil added WIP work in progress no changelog Pull request does not require a CHANGELOG entry and removed bug_fix labels Nov 12, 2024
    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 tests WIP work in progress
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant