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

API Rename base_estimator into estimator in ClassifierChain, and RegressorChain #29682

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dikraMasrour
Copy link

Closes #29620
This PR renames base_estimator into estimator in ClassifierChain, and RegressorChain such that they are consistent with other classifiers and regressors in the repository

Copy link

github-actions bot commented Aug 16, 2024

❌ Linting issues

This PR is introducing linting issues. Here's a summary of the issues. Note that you can avoid having linting issues by enabling pre-commit hooks. Instructions to enable them can be found here.

You can see the details of the linting issues under the lint job here


black

black detected issues. Please run black . locally and push the changes. Here you can see the detected issues. Note that running black might also fix some of the issues which might be detected by ruff. Note that the installed black version is black=24.3.0.


--- /home/runner/work/scikit-learn/scikit-learn/sklearn/multioutput.py	2024-08-16 13:16:56.829798+00:00
+++ /home/runner/work/scikit-learn/scikit-learn/sklearn/multioutput.py	2024-08-16 13:17:16.886115+00:00
@@ -642,11 +642,18 @@
             None,
         ],
     }
 
     def __init__(
-        self, estimator, *, order=None, cv=None, random_state=None, verbose=False, base_estimator="deprecated"
+        self,
+        estimator,
+        *,
+        order=None,
+        cv=None,
+        random_state=None,
+        verbose=False,
+        base_estimator="deprecated",
     ):
         self.estimator = estimator
         self.order = order
         self.cv = cv
         self.random_state = random_state
@@ -695,11 +702,11 @@
         inv_order = np.empty_like(self.order_)
         inv_order[self.order_] = np.arange(len(self.order_))
         Y_output = Y_output_chain[:, inv_order]
 
         return Y_output
-    
+
     def _validate_estimator(self, default=None):
         """Check the base estimator.
 
         Sets the `estimator_` attributes.
         """
@@ -736,11 +743,11 @@
     #     return self._estimator
 
     # # TODO: remove in 1.8
     # @property
     # def estimator_(self):
-    #     return self._estimator    
+    #     return self._estimator
 
     @abstractmethod
     def fit(self, X, Y, **fit_params):
         """Fit the model to data matrix X and targets Y.
 
@@ -761,11 +768,11 @@
         -------
         self : object
             Returns a fitted instance.
         """
         self._validate_estimator()
-        
+
         X, Y = self._validate_data(X, Y, multi_output=True, accept_sparse=True)
 
         random_state = check_random_state(self.random_state)
         self.order_ = self.order
         if isinstance(self.order_, tuple):
@@ -1038,22 +1045,21 @@
         order=None,
         cv=None,
         chain_method="predict",
         random_state=None,
         verbose=False,
-        base_estimator="deprecated"
+        base_estimator="deprecated",
     ):
         super().__init__(
             estimator,
             order=order,
             cv=cv,
             random_state=random_state,
             verbose=verbose,
-            base_estimator=base_estimator
+            base_estimator=base_estimator,
         )
         self.chain_method = chain_method
-        
 
     @_fit_context(
         # ClassifierChain.estimator is not validated yet
         prefer_skip_nested_validation=False
     )
@@ -1172,11 +1178,11 @@
 
     Parameters
     ----------
     estimator : estimator
         The base estimator from which the regressor chain is built.
-        
+
     base_estimator : estimator
         The base estimator from which the classifier chain is built.
 
         .. deprecated:: x.x
             `base_estimator` is deprecated and will be removed in x.x.
would reformat /home/runner/work/scikit-learn/scikit-learn/sklearn/multioutput.py

Oh no! 💥 💔 💥
1 file would be reformatted, 923 files would be left unchanged.

ruff

ruff detected issues. Please run ruff check --fix --output-format=full . locally, fix the remaining issues, and push the changes. Here you can see the detected issues. Note that the installed ruff version is ruff=0.5.1.


sklearn/multioutput.py:12:1: I001 [*] Import block is un-sorted or un-formatted
   |
12 | / from abc import ABCMeta, abstractmethod
13 | | from numbers import Integral
14 | | 
15 | | import numpy as np
16 | | import scipy.sparse as sp
17 | | import warnings
18 | | 
19 | | from .base import (
20 | |     BaseEstimator,
21 | |     ClassifierMixin,
22 | |     MetaEstimatorMixin,
23 | |     RegressorMixin,
24 | |     _fit_context,
25 | |     clone,
26 | |     is_classifier,
27 | | )
28 | | from .model_selection import cross_val_predict
29 | | from .utils import Bunch, check_random_state
30 | | from .utils._param_validation import HasMethods, StrOptions
31 | | from .utils._response import _get_response_values
32 | | from .utils._user_interface import _print_elapsed_time
33 | | from .utils.metadata_routing import (
34 | |     MetadataRouter,
35 | |     MethodMapping,
36 | |     _raise_for_params,
37 | |     _routing_enabled,
38 | |     process_routing,
39 | | )
40 | | from .utils.metaestimators import available_if
41 | | from .utils.multiclass import check_classification_targets
42 | | from .utils.parallel import Parallel, delayed
43 | | from .utils.validation import (
44 | |     _check_method_params,
45 | |     _check_response_method,
46 | |     check_is_fitted,
47 | |     has_fit_parameter,
48 | | )
49 | | 
50 | | __all__ = [
   | |_^ I001
51 |       "MultiOutputRegressor",
52 |       "MultiOutputClassifier",
   |
   = help: Organize imports

sklearn/multioutput.py:647:89: E501 Line too long (110 > 88)
    |
646 |     def __init__(
647 |         self, estimator, *, order=None, cv=None, random_state=None, verbose=False, base_estimator="deprecated"
    |                                                                                         ^^^^^^^^^^^^^^^^^^^^^^ E501
648 |     ):
649 |         self.estimator = estimator
    |

sklearn/multioutput.py:700:1: W293 [*] Blank line contains whitespace
    |
699 |         return Y_output
700 |     
    | ^^^^ W293
701 |     def _validate_estimator(self, default=None):
702 |         """Check the base estimator.
    |
    = help: Remove whitespace from blank line

sklearn/multioutput.py:731:89: E501 Line too long (90 > 88)
    |
729 |     # # mypy error: Decorated property not supported
730 |     # @deprecated(  # type: ignore
731 |     #     "Attribute `base_estimator_` was deprecated in version 1.6 and will be removed "
    |                                                                                         ^^ E501
732 |     #     "in 1.8. Use `estimator_` instead."
733 |     # )
    |

sklearn/multioutput.py:741:33: W291 [*] Trailing whitespace
    |
739 |     # @property
740 |     # def estimator_(self):
741 |     #     return self._estimator    
    |                                 ^^^^ W291
742 | 
743 |     @abstractmethod
    |
    = help: Remove trailing whitespace

sklearn/multioutput.py:766:1: W293 [*] Blank line contains whitespace
    |
764 |         """
765 |         self._validate_estimator()
766 |         
    | ^^^^^^^^ W293
767 |         X, Y = self._validate_data(X, Y, multi_output=True, accept_sparse=True)
    |
    = help: Remove whitespace from blank line

sklearn/multioutput.py:898:1: W293 Blank line contains whitespace
    |
896 |     estimator : estimator
897 |         The base estimator from which the classifier chain is built.
898 |             
    | ^^^^^^^^^^^^ W293
899 |     order : array-like of shape (n_outputs,) or 'random', default=None
900 |         If `None`, the order will be determined by the order of columns in
    |
    = help: Remove whitespace from blank line

sklearn/multioutput.py:952:1: W293 Blank line contains whitespace
    |
951 |         .. versionadded:: 1.2
952 |     
    | ^^^^ W293
953 |     base_estimator : object, default="deprecated"
954 |          Use `estimator` instead.
    |
    = help: Remove whitespace from blank line

sklearn/multioutput.py:1054:1: W293 [*] Blank line contains whitespace
     |
1052 |         )
1053 |         self.chain_method = chain_method
1054 |         
     | ^^^^^^^^ W293
1055 | 
1056 |     @_fit_context(
     |
     = help: Remove whitespace from blank line

sklearn/multioutput.py:1177:1: W293 Blank line contains whitespace
     |
1175 |     estimator : estimator
1176 |         The base estimator from which the regressor chain is built.
1177 |         
     | ^^^^^^^^ W293
1178 |     base_estimator : estimator
1179 |         The base estimator from which the classifier chain is built.
     |
     = help: Remove whitespace from blank line

Found 10 errors.
[*] 5 fixable with the `--fix` option (3 hidden fixes can be enabled with the `--unsafe-fixes` option).

mypy

mypy detected issues. Please fix them locally and push the changes. Here you can see the detected issues. Note that the installed mypy version is mypy=1.9.0.


sklearn/externals/_arff.py:782: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sklearn/utils/_testing.py:1038: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sklearn/utils/_testing.py:1039: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sklearn/utils/_testing.py:1040: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sklearn/multioutput.py:618: error: Name "_available_if_estimator_has" already defined on line 81  [no-redef]
Found 1 error in 1 file (checked 551 source files)

Generated for commit: e08127a. Link to the linter CI: here

@dikraMasrour dikraMasrour marked this pull request as draft August 16, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

base_estimator in Chain classes while estimator is the convention in Bagging and MultiOutput classes?
2 participants