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

Cleanup MrM settings #501

Merged
merged 5 commits into from
Oct 11, 2024
Merged

Cleanup MrM settings #501

merged 5 commits into from
Oct 11, 2024

Conversation

apchytr
Copy link
Collaborator

@apchytr apchytr commented Oct 10, 2024

User description

Description of the Change: Some cleanup of settings


PR Type

enhancement, tests


Description

  • Added a new MAX_BATCH_SIZE setting to the Settings class in mrmustard/utils/settings.py with a default value of 1000.
  • Cleaned up and reorganized the settings, including documentation improvements and reordering of properties.
  • Implemented a property for CACHE_DIR with appropriate getter and setter methods.
  • Refactored context management methods (__call__, __enter__, __exit__) for better usability.
  • Added a test in tests/test_utils/test_settings.py to ensure the MAX_BATCH_SIZE default value is correctly set.

Changes walkthrough 📝

Relevant files
Enhancement
settings.py
Add and document MAX_BATCH_SIZE setting in Mr Mustard       

mrmustard/utils/settings.py

  • Added a MAX_BATCH_SIZE setting with a default value of 1000.
  • Reorganized and cleaned up existing settings and their documentation.
  • Introduced a property for CACHE_DIR with getter and setter methods.
  • Refactored the __call__, __enter__, and __exit__ methods for context
    management.
  • +77/-74 
    Tests
    test_settings.py
    Add test for MAX_BATCH_SIZE default value                               

    tests/test_utils/test_settings.py

    • Added a test to verify the default value of MAX_BATCH_SIZE.
    +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 Oct 10, 2024
    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: 2 🔵🔵⚪⚪⚪
    🏅 Score: 92
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Organization
    The Settings class has grown large and might benefit from further organization or splitting into smaller, more focused classes.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Oct 10, 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
    Enhancement
    Use an Enum for BS_FOCK_METHOD to improve type safety and prevent invalid assignments

    Consider using an Enum for BS_FOCK_METHOD to restrict possible values and improve
    type safety. This would prevent accidental assignment of invalid values.

    mrmustard/utils/settings.py [84-85]

    -self.BS_FOCK_METHOD: str = "vanilla"  # can be 'vanilla' or 'schwinger'
    -r"""The method for computing a beam splitter in the Fock basis . Default is ``vanilla``."""
    +from enum import Enum, auto
     
    +class BSFockMethod(Enum):
    +    VANILLA = auto()
    +    SCHWINGER = auto()
    +
    +self.BS_FOCK_METHOD: BSFockMethod = BSFockMethod.VANILLA
    +r"""The method for computing a beam splitter in the Fock basis. Default is ``VANILLA``."""
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using an Enum for BS_FOCK_METHOD enhances type safety by restricting possible values to predefined options, reducing the risk of invalid assignments. This change is beneficial for maintaining code robustness and clarity.

    8
    Add input validation for MAX_BATCH_SIZE to ensure it's always a positive integer

    Consider adding input validation for the MAX_BATCH_SIZE setting to ensure it's
    always a positive integer. This can prevent potential issues with invalid values.

    mrmustard/utils/settings.py [81-82]

    -self.MAX_BATCH_SIZE: int = 1000
    +@property
    +def MAX_BATCH_SIZE(self) -> int:
    +    return self._max_batch_size
    +
    +@MAX_BATCH_SIZE.setter
    +def MAX_BATCH_SIZE(self, value: int):
    +    if not isinstance(value, int) or value <= 0:
    +        raise ValueError("MAX_BATCH_SIZE must be a positive integer")
    +    self._max_batch_size = value
    +
    +self._max_batch_size: int = 1000
     r"""The maximum batch size across all modes. Default is ``1000``."""
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding input validation for MAX_BATCH_SIZE ensures that it remains a positive integer, preventing potential runtime errors due to invalid values. This enhances the robustness and reliability of the code.

    7
    Use a more descriptive name for the MAX_BATCH_SIZE setting to clarify its scope

    Consider using a more descriptive name for MAX_BATCH_SIZE, such as
    MAX_TOTAL_BATCH_SIZE or MAX_BATCH_SIZE_ACROSS_MODES, to clearly indicate that it
    applies across all modes.

    mrmustard/utils/settings.py [81-82]

    -self.MAX_BATCH_SIZE: int = 1000
    -r"""The maximum batch size across all modes. Default is ``1000``."""
    +self.MAX_TOTAL_BATCH_SIZE: int = 1000
    +r"""The maximum total batch size across all modes. Default is ``1000``."""
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to rename MAX_BATCH_SIZE to a more descriptive name like MAX_TOTAL_BATCH_SIZE improves clarity by explicitly indicating its scope across all modes. This change enhances code readability but does not impact functionality.

    5

    💡 Need additional feedback ? start a PR chat

    Copy link

    codecov bot commented Oct 10, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 89.74%. Comparing base (d2a203d) to head (eb8129a).
    Report is 1 commits behind head on develop.

    Additional details and impacted files
    @@           Coverage Diff            @@
    ##           develop     #501   +/-   ##
    ========================================
      Coverage    89.74%   89.74%           
    ========================================
      Files          104      104           
      Lines         7623     7623           
    ========================================
      Hits          6841     6841           
      Misses         782      782           
    Files with missing lines Coverage Δ
    mrmustard/utils/settings.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 d2a203d...eb8129a. Read the comment docs.

    Copy link
    Collaborator

    @ziofil ziofil left a comment

    Choose a reason for hiding this comment

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

    Good idea. And thanks for sorting attributes and methods in Settings!

    @apchytr apchytr changed the title Adding max batch size to MrM settings Cleanup MrM settings Oct 11, 2024
    @apchytr apchytr requested a review from ziofil October 11, 2024 14:27
    Copy link
    Collaborator

    @ziofil ziofil left a comment

    Choose a reason for hiding this comment

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

    👍 thank you!

    @apchytr apchytr merged commit a75c1c2 into develop Oct 11, 2024
    7 checks passed
    @apchytr apchytr deleted the batchSize branch October 11, 2024 18:01
    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.

    3 participants