feat: Add functional API for algorithm contributtions #876
Conversation
Sourcery Code Quality Report❌ Merging this PR will decrease code quality in the affected files by 0.92%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Help us improve this quality report! |
|
Preview page for your plugin is ready here: |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #876 +/- ##
===========================================
+ Coverage 87.84% 90.88% +3.03%
===========================================
Files 151 196 +45
Lines 20484 30464 +9980
===========================================
+ Hits 17995 27688 +9693
- Misses 2489 2776 +287 ☔ View full report in Codecov by Sentry. |
Czaki
changed the title
|
Kudos, SonarCloud Quality Gate passed!
|
…ctional_plugin_api
…ctional_plugin_api
|
SonarCloud Quality Gate failed.
|
WalkthroughThe changes across various files reflect enhancements in a Python codebase, particularly in a CI/CD pipeline and algorithmic class definitions. New CI steps, class attributes, static methods, and decorators have been introduced, alongside improved error handling and type checking. These updates aim to streamline development workflows, enforce better coding practices, and extend functionality within the codebase. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- setup.cfg
Files selected for processing (11)
- .github/workflows/base_test_workflow.yml (1 hunks)
- package/PartSegCore/algorithm_describe_base.py (5 hunks)
- package/PartSegCore/io_utils.py (3 hunks)
- package/PartSegCore/segmentation/border_smoothing.py (1 hunks)
- package/PartSegCore/segmentation/noise_filtering.py (2 hunks)
- package/PartSegCore/segmentation/threshold.py (4 hunks)
- package/PartSegCore/segmentation/watershed.py (1 hunks)
- package/PartSegCore/utils.py (2 hunks)
- package/tests/test_PartSegCore/segmentation/test_threshold.py (3 hunks)
- package/tests/test_PartSegCore/test_algorithm_describe_base.py (4 hunks)
- package/tests/test_PartSegCore/test_utils.py (2 hunks)
Additional comments: 37
.github/workflows/base_test_workflow.yml (1)
- 115-120: The new "Upload coverage" step has been added to the workflow, which aligns with the PR objectives to enhance the CI/CD pipeline. However, ensure that the
coverage.xmlfile is generated before this step is executed, as the hunk does not show the generation of this file.package/PartSegCore/algorithm_describe_base.py (5)
16-25: The addition of imports and the
AlgorithmDescribeNotFoundexception class are correctly implemented and documented.120-126: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [122-130]
The changes to the
AlgorithmDescribeBaseMetaclass's__new__method correctly handle the new parameters and set class attributes based on the presence of abstract methods.
150-298: The new static methods and the updated
from_functionmethod in theAlgorithmDescribeBaseMetaclass are correctly implemented to support the creation of classes from functions.307-317: The
AlgorithmDescribeBaseclass has been correctly updated with a new__new__method and aget_doc_from_fieldsmethod to support class creation from functions and generate documentation.328-352: The
from_functionmethods in theAlgorithmDescribeBaseclass are correctly overloaded to handle different parameter sets for class generation from functions.package/PartSegCore/io_utils.py (4)
103-111: Docstrings added to
need_segmentationandneed_maskmethods improve code documentation.113-113: The summary mentions refactoring of
get_extensionsmethod using the walrus operator, but the provided hunk does not show this change.164-169: Refactoring of
get_extensionsmethod using the walrus operator inLoadBaseclass enhances readability.206-211: Modification in
load_metadata_baseto suppress the original exception could impact error handling and debugging.package/PartSegCore/segmentation/border_smoothing.py (3)
14-14: The addition of
method_from_fun="smooth"to theBaseSmoothingclass definition is consistent with the PR objectives to introduce a functional API for algorithm contributions. This change should be verified to ensure that it integrates correctly with the rest of the functional API infrastructure.15-15: The
__argument_class__attribute is set toBaseModelin theBaseSmoothingclass. Ensure that all subclasses ofBaseSmoothingthat require specific argument models override this attribute accordingly.17-17: The
smoothmethod inBaseSmoothingis correctly defined as an abstract class method, which enforces that all subclasses must provide their own implementation. This is a good practice for abstract base classes.package/PartSegCore/segmentation/noise_filtering.py (3)
3-3: The addition of the
abstractmethodimport is correct and necessary for the changes made to theNoiseFilteringBaseclass.29-30: The use of
@abstractmethoddecorator onnoise_filteris appropriate to ensure that subclasses implement this method.32-32: The subclasses of
NoiseFilteringBasecorrectly implement thenoise_filtermethod, adhering to the contract established by the abstract base class.package/PartSegCore/segmentation/threshold.py (4)
2-8: The import of
mahotasis correctly added and used within theriddler_calvardfunction.40-40: The
BaseThresholdclass now inherits fromAlgorithmDescribeBaseandABC, which aligns with the PR objective to support algorithm contributions through a functional API.279-280: The
MahotasThresholdclass does not inherit from any base class. This seems inconsistent with the design of other threshold classes in the file, which typically inherit fromBaseThresholdor another base class. Please verify if this is intentional or an oversight.283-323: The
riddler_calvardfunction is correctly implemented and registered inThresholdSelection, which is in line with the PR objectives.package/PartSegCore/segmentation/watershed.py (1)
- 25-25: The summary mentions the addition of
method_from_funas a keyword argument in theBaseWatershedclass definition, but the hunk shows it as a class attribute. This should be corrected in the summary or the code, depending on the intended implementation.package/PartSegCore/utils.py (6)
7-13: The addition of new imports seems appropriate for the new functionality introduced in the file.
468-546: The implementation of the
kwargs_to_modeldecorator and its helper functions appears to be correct and follows best practices for preserving function metadata and providing backward compatibility.468-484: The
_get_translation_dicts_from_signaturefunction correctly extracts translation dictionaries from the function signature and ensures that the parameters haveBaseModeltype annotations.487-493: The
_get_kwargs_from_old_argsfunction properly maps old positional arguments to keyword arguments based on the providedold_args_orderlist.496-505: The
_map_kwargs_to_modelfunction correctly maps keyword arguments to model instances, ensuring that each keyword argument corresponds to a field in the model.508-546: The
kwargs_to_modeldecorator is well-implemented, with clear error handling and a warning for deprecated usage. It ensures backward compatibility while encouraging users to update their function calls.package/tests/test_PartSegCore/segmentation/test_threshold.py (3)
15-21: The explicit specification of
dtype=np.uint32forsquareandcubearrays ensures that the data type is consistent and matches the expected type for the tests.42-42: The addition of type checking for
thr_infoto ensure it is an integer, float, ornp.integeris a good practice for data type validation in tests.55-56: The type checking for the elements of
thr_infointest_double_thresholdfunction ensures that both thresholds returned are of the correct type, which is a good practice for robust testing.package/tests/test_PartSegCore/test_algorithm_describe_base.py (5)
1-6: The addition of
ABCandabstractmethodimports suggests that abstract base classes and methods are being used, which is consistent with the PR's objective to enhance the codebase for algorithm contributions.10-16: The import statements are correctly formatted and relevant to the PR's objectives.
519-535: The introduction of
ClassForTestFromFuncBaseandClassForTestFromFuncwith abstract methods aligns with the PR's objective to provide a structured way for developers to contribute new algorithms.359-493: The new test cases added are comprehensive and appear to cover the functionality related to class generation from functions, which is a key part of the PR's objectives.
496-502: The test case for
ROIExtractionProfileis consistent with the PR's objectives and ensures that the new functional API for algorithm contributions is properly tested.package/tests/test_PartSegCore/test_utils.py (2)
13-18: The addition of
kwargs_to_modelto the import list is consistent with the PR objectives and the summary provided.367-395: The new test function
test_kwargs_to_modelis well-structured and covers various scenarios to ensure thekwargs_to_modeldecorator works as expected, including correct usage, deprecation warnings, and errors for incorrect usage.
| path: ./coverage.xml | ||
| retention-days: 5 |
There was a problem hiding this comment.
The retention period for the coverage.xml artifact is set to 5 days, which is different from the 7-day retention period for other artifacts. Confirm if this inconsistency is intentional.
|
SonarCloud Quality Gate failed.
|
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
setup.cfgis excluded by:!**/*.cfg
Files selected for processing (4)
- package/PartSegCore/algorithm_describe_base.py (5 hunks)
- package/PartSegCore/io_utils.py (5 hunks)
- package/PartSegCore/segmentation/algorithm_base.py (1 hunks)
- package/tests/test_PartSegCore/test_algorithm_describe_base.py (4 hunks)
Additional comments: 20
package/PartSegCore/segmentation/algorithm_base.py (2)
- 207-207: The method
get_info_textnow correctly specifies a return type ofstr, aligning with best practices for type hinting in Python.- 207-207: Ensure the
calculation_runmethod correctly specifies its return type asROIExtractionResultas per the summary. This change enhances type safety and code readability.Verification successful
The
calculation_runmethod correctly specifies its return type asROIExtractionResultin multiple instances within thePartSegCorepackage, as indicated by the search results. This confirms the implementation aligns with the summary's description, enhancing type safety and code readability.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the return type of calculation_run method ast-grep --lang python --pattern $'def calculation_run($_, $_) -> ROIExtractionResult { $$$ }'Length of output: 440
Script:
#!/bin/bash # Adjusting the pattern to find the calculation_run method without specifying parameters or return type ast-grep --lang python --pattern $'def calculation_run($$$) { $$$ }'Length of output: 524
Script:
#!/bin/bash # Using ripgrep to search for the calculation_run method in Python files rg "def calculation_run" --type pyLength of output: 2491
package/PartSegCore/io_utils.py (4)
- 71-71: Adding a return type of
strto theget_short_namemethod inSaveBaseandLoadBaseclasses improves type clarity and consistency.- 105-105: The addition of documentation for the
need_segmentationmethod clarifies its purpose and usage, enhancing code readability.- 110-110: Similarly, adding documentation to the
need_maskmethod improves understanding of its necessity in certain save operations.- 166-166: The use of the walrus operator in
get_extensionsmethod simplifies the code and improves readability by avoiding an extra line of code for the match operation.package/tests/test_PartSegCore/test_algorithm_describe_base.py (14)
- 3-3: The import of
ABCandabstractmethodis necessary for defining abstract base classes and methods, aligning with Python's way of enforcing class interfaces.- 13-13: The introduction of
AlgorithmDescribeBaseMetain the imports suggests enhancements to metaclass functionalities, potentially for dynamic class creation or attribute enforcement.- 359-377: The tests for generating classes from functions validate the functionality and error handling of the
from_functionmethod, ensuring robustness and correct behavior.- 379-385: The test for missing return annotation correctly checks for runtime errors when abstract methods in subclasses do not specify return types, enforcing good coding practices.
- 387-398: This test validates that the
from_functionmethod raises an appropriate error when used with classes that do not support it, ensuring that the method's usage is restricted to compatible classes.- 400-405: The test for passing incorrect types to the
from_functionmethod ensures type safety and correct parameter handling, contributing to the robustness of the class generation feature.- 407-424: The comprehensive test for the
from_functionmethod demonstrates its capability to dynamically create classes from functions, validating the feature's core functionality.- 426-435: Testing the
from_functiondecorator without explicit parameters ensures flexibility and ease of use in class generation, highlighting the feature's adaptability.- 437-442: This test checks for errors when additional, undefined parameters are passed to the
from_functionmethod, ensuring strict parameter validation.- 444-449: The test for positional-only arguments in the
from_functionmethod ensures compatibility with Python's function signature restrictions, maintaining consistency in method definitions.- 451-472: Testing the
from_functionmethod as a decorator on a class with abstract methods validates its versatility and integration with Python's class system.- 474-485: The test for class generation without user-provided attributes through the
from_functiondecorator demonstrates the method's ability to handle minimalistic function definitions.- 487-493: Testing the handling of keyword arguments in functions used with
from_functionensures that dynamically generated classes can accept and process variable arguments.- 496-500: The test for
ROIExtractionProfileinstantiation and the use ofpytest.warnsto check for deprecation warnings ensures backward compatibility and future-proofing of the API.
| ): | ||
| raise RuntimeError("class need to have __argument_class__ set or get_fields functions defined") | ||
| cls2.__new_style__ = getattr(cls2.get_fields, "__is_partial_abstractmethod__", False) | ||
| cls2.__from_function__ = getattr(cls2, "__from_function__", False) | ||
| cls2.__abstract_getters__ = {} | ||
| cls2.__method_name__ = method_from_fun or getattr(cls2, "__method_name__", None) | ||
| cls2.__additional_parameters_name__ = additional_parameters or getattr( | ||
| cls2, "__additional_parameters_name__", None | ||
| ) | ||
| if cls2.__additional_parameters_name__ is None: | ||
| cls2.__additional_parameters_name__ = cls._get_calculation_method_params_name(cls2) | ||
|
|
||
| cls2.__support_from_function__ = ( | ||
| cls2.__method_name__ is not None and cls2.__additional_parameters_name__ is not None | ||
| ) | ||
|
|
||
| cls2.__abstract_getters__, cls2.__support_from_function__ = cls._get_abstract_getters( | ||
| cls2, cls2.__support_from_function__, method_from_fun | ||
| ) | ||
| return cls2 | ||
|
|
||
| @staticmethod | ||
| def _get_abstract_getters( | ||
| cls2, support_from_function, calculation_method | ||
| ) -> typing.Tuple[typing.Dict[str, typing.Any], bool]: | ||
| abstract_getters = {} | ||
| if hasattr(cls2, "__abstractmethods__") and cls2.__abstractmethods__: | ||
| # get all abstract methods that starts with `get_` | ||
| for method_name in cls2.__abstractmethods__: | ||
| if method_name.startswith("get_"): | ||
| method = getattr(cls2, method_name) | ||
| if "return" not in method.__annotations__: | ||
| msg = f"Method {method_name} of {cls2.__qualname__} need to have return type defined" | ||
| try: | ||
| file_name = inspect.getsourcefile(method) | ||
| line = inspect.getsourcelines(method)[1] | ||
| msg += f" in {file_name}:{line}" | ||
| except TypeError: | ||
| pass | ||
| raise RuntimeError(msg) | ||
|
|
||
| abstract_getters[method_name[4:]] = getattr(cls2, method_name).__annotations__["return"] | ||
| elif method_name != calculation_method: | ||
| support_from_function = False | ||
| return abstract_getters, support_from_function | ||
|
|
||
| @staticmethod | ||
| def _get_calculation_method_params_name(cls2) -> typing.Optional[str]: | ||
| if cls2.__method_name__ is None: | ||
| return None | ||
| signature = inspect.signature(getattr(cls2, cls2.__method_name__)) | ||
| if "arguments" in signature.parameters: | ||
| return "arguments" | ||
| if "params" in signature.parameters: | ||
| return "params" | ||
| if "parameters" in signature.parameters: | ||
| return "parameters" | ||
| raise RuntimeError(f"Cannot determine arguments parameter name in {cls2.__method_name__}") | ||
|
|
||
| @staticmethod | ||
| def _validate_if_all_abstract_getters_are_defined(abstract_getters, kwargs): | ||
| abstract_getters_set = set(abstract_getters) | ||
| kwargs_set = set(kwargs.keys()) | ||
|
|
||
| if abstract_getters_set != kwargs_set: | ||
| # Provide a nice error message with information about what is missing and is obsolete | ||
| missing_text = ", ".join(sorted(abstract_getters_set - kwargs_set)) | ||
| if missing_text: | ||
| missing_text = f"Not all abstract methods are provided, missing: {missing_text}." | ||
| else: | ||
| missing_text = "" | ||
| extra_text = ", ".join(sorted(kwargs_set - abstract_getters_set)) | ||
| if extra_text: | ||
| extra_text = f"There are extra attributes in call: {extra_text}." | ||
| else: | ||
| extra_text = "" | ||
|
|
||
| raise ValueError(f"{missing_text} {extra_text}") | ||
|
|
||
| @staticmethod | ||
| def _validate_function_parameters(func, method, method_name) -> set: | ||
| """ | ||
| Validate if all parameters without default values are defined in self.__calculation_method__ | ||
| :param func: function to validate | ||
| :return: set of parameters that should be dropped | ||
| """ | ||
| signature = inspect.signature(func) | ||
| base_method_signature = inspect.signature(method) | ||
| take_all = False | ||
|
|
||
| for parameter in signature.parameters.values(): | ||
| if parameter.kind in {inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.POSITIONAL_ONLY}: | ||
| raise ValueError(f"Function {func} should not have positional only parameters") | ||
| if ( | ||
| parameter.default is inspect.Parameter.empty | ||
| and parameter.name not in base_method_signature.parameters | ||
| and parameter.kind != inspect.Parameter.VAR_KEYWORD | ||
| ): | ||
| raise ValueError(f"Parameter {parameter.name} is not defined in {method_name} method") | ||
|
|
||
| if parameter.kind == inspect.Parameter.VAR_KEYWORD: | ||
| take_all = True | ||
|
|
||
| if take_all: | ||
| return set() | ||
|
|
||
| return { | ||
| parameters.name | ||
| for parameters in base_method_signature.parameters.values() | ||
| if parameters.name not in signature.parameters | ||
| } | ||
|
|
||
| @staticmethod | ||
| def _get_argument_class_from_signature(func, argument_name: str): | ||
| signature = inspect.signature(func) | ||
| if argument_name not in signature.parameters: | ||
| return BaseModel | ||
| return signature.parameters[argument_name].annotation | ||
|
|
||
| @staticmethod | ||
| def _get_parameters_from_signature(func): | ||
| signature = inspect.signature(func) | ||
| return [parameters.name for parameters in signature.parameters.values()] | ||
|
|
||
| def from_function(self, func=None, **kwargs): | ||
| """generate new class from function""" | ||
|
|
||
| # Test if all abstract methods values are provided in kwargs | ||
|
|
||
| if not self.__support_from_function__: | ||
| raise RuntimeError("This class does not support from_function method") | ||
|
|
||
| self._validate_if_all_abstract_getters_are_defined(self.__abstract_getters__, kwargs) | ||
|
|
||
| # check if all values have correct type | ||
| for key, value in kwargs.items(): | ||
| if not isinstance(value, self.__abstract_getters__[key]): | ||
| raise TypeError(f"Value for {key} should be {self.__abstract_getters__[key]}") | ||
|
|
||
| def _getter_by_name(name): | ||
| def _func(): | ||
| return kwargs[name] | ||
|
|
||
| return _func | ||
|
|
||
| parameters_order = self._get_parameters_from_signature(getattr(self, self.__method_name__)) | ||
|
|
||
| def _class_generator(func_): | ||
| drop_attr = self._validate_function_parameters( | ||
| func_, getattr(self, self.__method_name__), self.__method_name__ | ||
| ) | ||
|
|
||
| @wraps(func_) | ||
| def _calculate_method(*args, **kwargs_): | ||
| for attr, name in zip(args, parameters_order): | ||
| if name in kwargs_: | ||
| raise ValueError(f"Parameter {name} is defined twice") | ||
| kwargs_[name] = attr | ||
|
|
||
| for name in drop_attr: | ||
| kwargs_.pop(name, None) | ||
| return func_(**kwargs_) | ||
|
|
||
| class_dkt = {f"get_{name}": _getter_by_name(name) for name in self.__abstract_getters__} | ||
|
|
||
| class_dkt[self.__method_name__] = _calculate_method | ||
| class_dkt["__argument_class__"] = self._get_argument_class_from_signature( | ||
| func_, self.__additional_parameters_name__ | ||
| ) | ||
| class_dkt["__from_function__"] = True | ||
|
|
||
| return type(func_.__name__.replace("_", " ").title().replace(" ", ""), (self,), class_dkt) | ||
|
|
||
| if func is None: | ||
| return _class_generator | ||
| return _class_generator(func) | ||
|
|
||
|
|
||
| class AlgorithmDescribeBase(ABC, metaclass=AlgorithmDescribeBaseMeta): | ||
| """ |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [123-147]
The modifications to the AlgorithmDescribeBaseMeta metaclass introduce new parameters method_from_fun and additional_parameters to the __new__ method. These changes are designed to support the creation of classes from functions, enhancing the flexibility of algorithm definitions. However, there's a potential issue with the logic that determines whether a class supports the from_function method based on the presence of __method_name__ and __additional_parameters_name__. This logic may not accurately reflect all scenarios where from_function could be applicable or beneficial. Additionally, the error handling in the absence of __argument_class__ or get_fields functions could be more descriptive, providing guidance on how to resolve the issue.
Consider refining the logic that determines the support for from_function to cover more scenarios and improve the error message when __argument_class__ or get_fields functions are missing to guide developers on how to make their classes compatible.
| @staticmethod | ||
| def _get_abstract_getters( | ||
| cls2, support_from_function, calculation_method | ||
| ) -> typing.Tuple[typing.Dict[str, typing.Any], bool]: | ||
| abstract_getters = {} | ||
| if hasattr(cls2, "__abstractmethods__") and cls2.__abstractmethods__: | ||
| # get all abstract methods that starts with `get_` | ||
| for method_name in cls2.__abstractmethods__: | ||
| if method_name.startswith("get_"): | ||
| method = getattr(cls2, method_name) | ||
| if "return" not in method.__annotations__: | ||
| msg = f"Method {method_name} of {cls2.__qualname__} need to have return type defined" | ||
| try: | ||
| file_name = inspect.getsourcefile(method) | ||
| line = inspect.getsourcelines(method)[1] | ||
| msg += f" in {file_name}:{line}" | ||
| except TypeError: | ||
| pass | ||
| raise RuntimeError(msg) | ||
|
|
||
| abstract_getters[method_name[4:]] = getattr(cls2, method_name).__annotations__["return"] | ||
| elif method_name != calculation_method: | ||
| support_from_function = False | ||
| return abstract_getters, support_from_function |
There was a problem hiding this comment.
The _get_abstract_getters method has been modified to support the new functionality related to creating classes from functions. This method now checks for abstract methods starting with get_ and validates their return type annotations. While this approach ensures that abstract getters are properly defined, it may be too restrictive by excluding potentially valid abstract methods that do not follow this naming convention. Additionally, the error message generated when a return type is not defined could be enhanced to provide more specific guidance on how to correct the issue.
Consider allowing more flexibility in the naming convention of abstract methods and improve the error message for missing return type annotations to offer clearer guidance on resolving the issue.
| @staticmethod | ||
| def _get_calculation_method_params_name(cls2) -> typing.Optional[str]: | ||
| if cls2.__method_name__ is None: | ||
| return None | ||
| signature = inspect.signature(getattr(cls2, cls2.__method_name__)) | ||
| if "arguments" in signature.parameters: | ||
| return "arguments" | ||
| if "params" in signature.parameters: | ||
| return "params" | ||
| if "parameters" in signature.parameters: | ||
| return "parameters" | ||
| raise RuntimeError(f"Cannot determine arguments parameter name in {cls2.__method_name__}") |
There was a problem hiding this comment.
The _get_calculation_method_params_name method attempts to determine the name of the parameter that represents arguments in the calculation method. This method assumes specific parameter names (arguments, params, parameters) and raises a runtime error if none are found. This approach may not accommodate all valid parameter naming conventions used by developers, potentially limiting the flexibility of the API.
Enhance the method to support a wider range of parameter naming conventions or provide a mechanism for developers to specify the parameter name explicitly, improving the API's flexibility.
| @staticmethod | ||
| def _validate_if_all_abstract_getters_are_defined(abstract_getters, kwargs): | ||
| abstract_getters_set = set(abstract_getters) | ||
| kwargs_set = set(kwargs.keys()) | ||
|
|
||
| if abstract_getters_set != kwargs_set: | ||
| # Provide a nice error message with information about what is missing and is obsolete | ||
| missing_text = ", ".join(sorted(abstract_getters_set - kwargs_set)) | ||
| if missing_text: | ||
| missing_text = f"Not all abstract methods are provided, missing: {missing_text}." | ||
| else: | ||
| missing_text = "" | ||
| extra_text = ", ".join(sorted(kwargs_set - abstract_getters_set)) | ||
| if extra_text: | ||
| extra_text = f"There are extra attributes in call: {extra_text}." | ||
| else: | ||
| extra_text = "" | ||
|
|
||
| raise ValueError(f"{missing_text} {extra_text}") |
There was a problem hiding this comment.
The _validate_if_all_abstract_getters_are_defined method checks if all required abstract getters are provided in kwargs. While this validation is crucial for ensuring that all necessary information is provided, the error message generated when there are missing or extra attributes could be more actionable. Specifically, it could guide developers on how to correct the issue by providing examples or suggesting where to define the missing getters.
Improve the error message generated by _validate_if_all_abstract_getters_are_defined to include actionable guidance on how to resolve issues with missing or extra attributes.
| @staticmethod | ||
| def _validate_function_parameters(func, method, method_name) -> set: | ||
| """ | ||
| Validate if all parameters without default values are defined in self.__calculation_method__ | ||
| :param func: function to validate | ||
| :return: set of parameters that should be dropped | ||
| """ | ||
| signature = inspect.signature(func) | ||
| base_method_signature = inspect.signature(method) | ||
| take_all = False | ||
|
|
||
| for parameter in signature.parameters.values(): | ||
| if parameter.kind in {inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.POSITIONAL_ONLY}: | ||
| raise ValueError(f"Function {func} should not have positional only parameters") | ||
| if ( | ||
| parameter.default is inspect.Parameter.empty | ||
| and parameter.name not in base_method_signature.parameters | ||
| and parameter.kind != inspect.Parameter.VAR_KEYWORD | ||
| ): | ||
| raise ValueError(f"Parameter {parameter.name} is not defined in {method_name} method") | ||
|
|
||
| if parameter.kind == inspect.Parameter.VAR_KEYWORD: | ||
| take_all = True | ||
|
|
||
| if take_all: | ||
| return set() | ||
|
|
||
| return { | ||
| parameters.name | ||
| for parameters in base_method_signature.parameters.values() |
There was a problem hiding this comment.
The _validate_function_parameters method validates if all parameters without default values are defined in the calculation method. This validation is essential for ensuring compatibility between the function and the class it is being converted into. However, the method currently raises a generic ValueError for missing parameters, which may not provide enough context for developers to understand the source of the error and how to fix it.
Enhance the error handling in _validate_function_parameters to provide more specific and actionable error messages, helping developers identify and correct issues with missing parameters more efficiently.
| def from_function(self, func=None, **kwargs): | ||
| """generate new class from function""" | ||
|
|
||
| # Test if all abstract methods values are provided in kwargs | ||
|
|
||
| if not self.__support_from_function__: | ||
| raise RuntimeError("This class does not support from_function method") | ||
|
|
||
| self._validate_if_all_abstract_getters_are_defined(self.__abstract_getters__, kwargs) | ||
|
|
||
| # check if all values have correct type | ||
| for key, value in kwargs.items(): | ||
| if not isinstance(value, self.__abstract_getters__[key]): | ||
| raise TypeError(f"Value for {key} should be {self.__abstract_getters__[key]}") | ||
|
|
||
| def _getter_by_name(name): | ||
| def _func(): | ||
| return kwargs[name] | ||
|
|
||
| return _func | ||
|
|
||
| parameters_order = self._get_parameters_from_signature(getattr(self, self.__method_name__)) | ||
|
|
||
| def _class_generator(func_): | ||
| drop_attr = self._validate_function_parameters( | ||
| func_, getattr(self, self.__method_name__), self.__method_name__ | ||
| ) | ||
|
|
||
| @wraps(func_) | ||
| def _calculate_method(*args, **kwargs_): | ||
| for attr, name in zip(args, parameters_order): | ||
| if name in kwargs_: | ||
| raise ValueError(f"Parameter {name} is defined twice") | ||
| kwargs_[name] = attr | ||
|
|
||
| for name in drop_attr: | ||
| kwargs_.pop(name, None) | ||
| return func_(**kwargs_) | ||
|
|
||
| class_dkt = {f"get_{name}": _getter_by_name(name) for name in self.__abstract_getters__} | ||
|
|
||
| class_dkt[self.__method_name__] = _calculate_method | ||
| class_dkt["__argument_class__"] = self._get_argument_class_from_signature( | ||
| func_, self.__additional_parameters_name__ | ||
| ) | ||
| class_dkt["__from_function__"] = True | ||
|
|
||
| return type(func_.__name__.replace("_", " ").title().replace(" ", ""), (self,), class_dkt) | ||
|
|
||
| if func is None: | ||
| return _class_generator | ||
| return _class_generator(func) |
There was a problem hiding this comment.
The from_function method in the AlgorithmDescribeBaseMeta class is a significant addition, enabling the generation of new classes from functions. This method includes comprehensive validation and error handling to ensure that the function and provided arguments meet the necessary criteria. However, the method's complexity and the multiple nested functions could impact readability and maintainability. Additionally, the method assumes that the function's name follows a specific naming convention (func_.__name__.replace("_", " ").title().replace(" ", "")), which may not always be desirable or applicable.
Consider refactoring the from_function method to simplify its structure and improve readability. Additionally, provide a way for developers to specify the class name explicitly when it cannot be appropriately inferred from the function's name.
| def __new__(cls, *args, **kwargs): | ||
| if cls.__from_function__: | ||
| return getattr(cls, cls.__method_name__)(*args, **kwargs) | ||
| return super().__new__(cls) |
There was a problem hiding this comment.
The __new__ method in the AlgorithmDescribeBase class has been modified to support instances created from functions. This modification allows for dynamic behavior based on whether the class was generated from a function. While this approach adds flexibility, it also introduces complexity into the instantiation process, potentially affecting the predictability and debuggability of classes derived from AlgorithmDescribeBase.
Evaluate the impact of the modified __new__ method on the predictability and debuggability of classes derived from AlgorithmDescribeBase. Consider providing additional documentation or examples to help developers understand and effectively use this new functionality.
| @classmethod | ||
| def from_function( | ||
| cls: TypeT, func=None, **kwargs | ||
| ) -> typing.Union[TypeT, typing.Callable[[typing.Callable], TypeT]]: | ||
| def _from_function(func_) -> typing.Type["AlgorithmDescribeBase"]: | ||
| if "name" not in kwargs: | ||
| kwargs["name"] = func_.__name__.replace("_", " ").title() | ||
| return AlgorithmDescribeBaseMeta.from_function(cls, func_, **kwargs) | ||
|
|
||
| if func is None: | ||
| return _from_function | ||
| return _from_function(func) |
There was a problem hiding this comment.
The from_function class method in the AlgorithmDescribeBase class provides an overload mechanism to support different usage patterns (with or without a specified function). This method leverages the AlgorithmDescribeBaseMeta.from_function to generate new classes from functions, demonstrating a clear separation of concerns between the metaclass and the class itself. However, the documentation could be improved to better explain the method's overloads and provide examples of how to use each pattern effectively.
Enhance the documentation for the from_function class method in AlgorithmDescribeBase to include examples and explanations of the different usage patterns supported by the method's overloads.
| decoded_data = json.loads(str(data), object_hook=partseg_object_hook) | ||
| except Exception: # pragma: no cover | ||
| raise e # noqa: B904 | ||
| raise e from None |
There was a problem hiding this comment.
Modifying exception handling in load_metadata_base to use raise e from None suppresses the context of the original exception, which might hinder debugging. Consider if this change aligns with the desired error handling behavior.
- raise e from None
+ raise eCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| raise e from None | |
| raise e |
Summary by CodeRabbit
New Features
AlgorithmDescribeBaseMetaclass to support additional parameters and methods for class creation from functions.Enhancements
need_segmentationandneed_maskmethods.get_extensionsmethod for better readability and efficiency.load_metadata_basefunction to provide clearer error handling.Bug Fixes
BaseSmoothing,NoiseFilteringBase,BaseThreshold, andBaseWatershedto correctly incorporate new method attributes.Documentation
Tests