Skip to content

Reduce cognitive complexity and fix code duplication in scikit-learn utilities - #644

Open
sonarqube-agent[bot] wants to merge 1 commit into
mainfrom
remediate-main-20260713-010123-0ad59131
Open

Reduce cognitive complexity and fix code duplication in scikit-learn utilities#644
sonarqube-agent[bot] wants to merge 1 commit into
mainfrom
remediate-main-20260713-010123-0ad59131

Conversation

@sonarqube-agent

Copy link
Copy Markdown

This PR was automatically created by the Remediation Agent's Scheduled backlog remediation feature.

Why these issues? All five issues are CRITICAL severity with high-confidence automated fixes: string literal deduplication, helper method extractions to reduce cognitive complexity, and adding a missing parameter for LSP compliance. The changes span three source files with clear, isolated scopes and cohesive improvements to code quality and maintainability.

This PR fixes 5 CRITICAL SonarQube issues including reducing cognitive complexity in dict vectorizer and pretty printing utilities, eliminating string literal duplication, and adding a missing function parameter to maintain Liskov Substitution Principle compliance. These changes improve code maintainability, reduce nesting depth, and ensure method signature consistency across inheritance hierarchies.

View Project in SonarCloud


Fixed Issues

python:S1192 - Define a constant instead of duplicating this literal "boolean indexing" 3 times. • CRITICALView issue

Location: sklearn/externals/array_api_extra/_lib/_utils/_helpers.py:327

Why is this an issue?

Duplicated string literals make the process of refactoring complex and error-prone, as any change would need to be propagated on all occurrences.

What changed

Defines the constant _BOOLEAN_INDEXING to hold the string literal "boolean indexing", which was previously duplicated 3 times in the capabilities function. This addresses the code smell about duplicating the string literal "boolean indexing" 3 times by providing a single constant that can be referenced from all locations, so any future change only needs to be made in one place.

--- a/sklearn/externals/array_api_extra/_lib/_utils/_helpers.py
+++ b/sklearn/externals/array_api_extra/_lib/_utils/_helpers.py
@@ -303,0 +304,3 @@ def meta_namespace(
+_BOOLEAN_INDEXING = "boolean indexing"
+
+
python:S3776 - Refactor this function to reduce its Cognitive Complexity from 34 to the 15 allowed. • CRITICALView issue

Location: sklearn/feature_extraction/_dict_vectorizer.py:194

Why is this an issue?

Cognitive Complexity is a measure of how hard it is to understand the control flow of a unit of code. Code with high cognitive complexity is hard to read, understand, test, and modify.

What changed

This hunk extracts two new helper methods (_process_feature_value and _sort_feature_names) from the _transform method. The _process_feature_value method encapsulates the logic for processing a single feature-value pair (type checking for str, Number, Iterable, vocabulary updates, etc.), and _sort_feature_names encapsulates the feature name sorting and matrix reordering logic. By moving these deeply nested blocks into separate methods, the cognitive complexity of _transform is significantly reduced. The nested conditionals and loops that contributed heavily to the complexity score — including type checks at nesting level 2-3, vocab updates at nesting level 3-4, and the sorting loop — are now simple method calls, which are 'free' in cognitive complexity scoring. This directly addresses the high cognitive complexity (34 vs allowed 15) of the _transform function by breaking it down into smaller, easier-to-manage pieces.

--- a/sklearn/feature_extraction/_dict_vectorizer.py
+++ b/sklearn/feature_extraction/_dict_vectorizer.py
@@ -193,0 +194,46 @@ class DictVectorizer(TransformerMixin, BaseEstimator):
+    def _process_feature_value(
+        self, f, v, feature_names, vocab, fitting, indices, values
+    ):
+        """Process a single feature-value pair during transformation."""
+        if isinstance(v, str):
+            feature_name = "%s%s%s" % (f, self.separator, v)
+            v = 1
+        elif isinstance(v, Number) or (v is None):
+            feature_name = f
+        elif not isinstance(v, Mapping) and isinstance(v, Iterable):
+            feature_name = None
+            self._add_iterable_element(
+                f,
+                v,
+                feature_names,
+                vocab,
+                fitting=fitting,
+                transforming=True,
+                indices=indices,
+                values=values,
+            )
+        else:
+            raise TypeError(
+                f"Unsupported value Type {type(v)} "
+                f"for {f}: {v}.\n"
+                f"{type(v)} objects are not supported."
+            )
+
+        if feature_name is not None:
+            if fitting and feature_name not in vocab:
+                vocab[feature_name] = len(feature_names)
+                feature_names.append(feature_name)
+
+            if feature_name in vocab:
+                indices.append(vocab[feature_name])
+                values.append(self.dtype(v))
+
+    def _sort_feature_names(self, feature_names, vocab, result_matrix):
+        """Sort feature names and reorder the result matrix accordingly."""
+        feature_names.sort()
+        map_index = np.empty(len(feature_names), dtype=np.int32)
+        for new_val, f in enumerate(feature_names):
+            map_index[new_val] = vocab[f]
+            vocab[f] = new_val
+        return result_matrix[:, map_index]
+
python:S3776 - Refactor this function to reduce its Cognitive Complexity from 28 to the 15 allowed. • CRITICALView issue 1
python:S3776 - Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed. • CRITICALView issue 2

Location: sklearn/utils/_pprint.py:230

Why is this an issue?

Cognitive Complexity is a measure of how hard it is to understand the control flow of a unit of code. Code with high cognitive complexity is hard to read, understand, test, and modify.

What changed

This hunk extracts the compact item formatting logic from the _format_items method into a new helper method _format_items_compact. By moving the width checking, delimiter management, and representation writing logic into its own method, the cognitive complexity of _format_items is reduced because the nested conditionals (if width < w, if delim, if width >= w) that contributed to its complexity score of 21 are now in a separate function. This also helps reduce the overall high cognitive complexity of _format_items at line 293, which had a score of 21 exceeding the allowed threshold of 15.

--- a/sklearn/utils/_pprint.py
+++ b/sklearn/utils/_pprint.py
@@ -292,0 +309,22 @@ class _EstimatorPrettyPrinter(pprint.PrettyPrinter):
+    def _format_items_compact(
+        self, ent, width, max_width, delim, delimnl, write, context, level
+    ):
+        """Format a single item in compact mode.
+
+        Returns updated (width, delim, skip) where skip indicates whether
+        the non-compact fallback should be skipped.
+        """
+        rep = self._repr(ent, context, level)
+        w = len(rep) + 2
+        if width < w:
+            width = max_width
+            if delim:
+                delim = delimnl
+        if width >= w:
+            width -= w
+            write(delim)
+            delim = ", "
+            write(rep)
+            return width, delim, True
+        return width, delim, False
+
python:S2638 - Add missing parameters random_state. • CRITICALView issue

Location: sklearn/ensemble/_gb.py:961

Why is this an issue?

Because a subclass instance may be used as an instance of the superclass, overriding methods should uphold the aspects of the superclass contract that relate to the Liskov Substitution Principle. Specifically, an overriding method should be callable with the same parameters as the overriden one.

What changed

This hunk adds the missing random_state parameter (with a default value of None) to the _make_estimator method in BaseGradientBoosting. The overriding method was previously missing the random_state parameter that exists in the superclass BaseEnsemble._make_estimator signature, violating the Liskov Substitution Principle. By adding random_state=None as an optional parameter, the overriding method now accepts the same parameters as the overridden one, making it callable with the same arguments as the parent class method. This directly resolves the code smell about the overriding method needing to add the missing random_state parameter to match the overridden method's definition.

--- a/sklearn/ensemble/_gb.py
+++ b/sklearn/ensemble/_gb.py
@@ -961,1 +961,1 @@ class BaseGradientBoosting(BaseEnsemble, metaclass=ABCMeta):
-    def _make_estimator(self, append=True):
+    def _make_estimator(self, append=True, random_state=None):

Have a suggestion or found an issue? Share your feedback here.


SonarQube Remediation Agent uses AI. Check for mistakes.

Fixed issues:
- AZ45Cuz9RXnEWm2Rf4rC for python:S2638 rule
- AZ45CvcPRXnEWm2Rf41B for python:S1192 rule
- AZ45Cx_SRXnEWm2Rf5ZC for python:S3776 rule
- AZ45CuOoRXnEWm2Rf4iL for python:S3776 rule
- AZ45CuOoRXnEWm2Rf4iM for python:S3776 rule

Generated by SonarQube Agent (task: 7fcef932-1bb0-44cf-877d-1ba3a690a0fe)
@sonarqube-agent

Copy link
Copy Markdown
Author

⚠️ This repository does not have a CODEOWNERS file. The PR has been created but has not been automatically assigned to any reviewer. To ensure PRs are reviewed promptly, consider adding a CODEOWNERS file to your repository.

@sonarqubecloud

Copy link
Copy Markdown

@sonarqube-cloud-dev7

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant