Cleanup MrM settings - Githubissues

apchytr commented 1 month ago

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

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: 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.

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:

Category	Suggestion	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]](https://github.com/XanaduAI/MrMustard/pull/501/files#diff-eaa5467c3f570ae6efabf33215bd7a36eb842dcec72aa1799c3bfb43b6e9f6b7R84-R85) ```diff -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]](https://github.com/XanaduAI/MrMustard/pull/501/files#diff-eaa5467c3f570ae6efabf33215bd7a36eb842dcec72aa1799c3bfb43b6e9f6b7R81-R82) ```diff -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]](https://github.com/XanaduAI/MrMustard/pull/501/files#diff-eaa5467c3f570ae6efabf33215bd7a36eb842dcec72aa1799c3bfb43b6e9f6b7R81-R82) ```diff -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

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ 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](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/501?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI) | Coverage Δ | | |---|---|---| | [mrmustard/utils/settings.py](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/501?src=pr&el=tree&filepath=mrmustard%2Futils%2Fsettings.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI#diff-bXJtdXN0YXJkL3V0aWxzL3NldHRpbmdzLnB5) | `100.00% <100.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/501?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/501?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XanaduAI). Last update [d2a203d...eb8129a](https://app.codecov.io/gh/XanaduAI/MrMustard/pull/501?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).

XanaduAI / MrMustard

Cleanup MrM settings #501