Refactor BaseEstimator's and Kernel's get_params and set_params #14175

alegonz · 2019-06-24T14:12:37Z

Reference Issues/PRs

Hi there, this is a PR for #13555 .

What does this implement/fix? Explain your changes.

Moved _get_param_names out of BaseEstimator as utility function now called get_param_names_from_constructor.
- This is meant for reuse in gaussian_process.kernels.Kernel .
- Added a SignatureError class to pass the signature from the utility function up to the appropriate exception message in BaseEstimator and gaussian_process.kernels.Kernel.
Modified gaussian_process.kernels.Kernel to use the new utility function and remove duplication.
Changed GenericUnivariateSelect._make_selector to use get_params instead of the new utility function so that it raises the proper error in case of a selector with inappropriate signature.
Also refactored get_params to call from a new _get_params_from method. This is so get_params can be easily overridden to take a signature of, for example, the parent class in custom subclasses of client code.

Any other comments?

Docstrings will be added later.
Other that raising SignatureError (subclassed from RuntimeError) these changes don't add or change current behavior, so I haven't added any new tests. If new tests might seem necessary, please let me know.

jnothman

Thanks. There's a lot going on, so I hope you can accept a slow review!

jnothman · 2019-06-25T23:59:22Z

sklearn/base.py

+        try:
+            param_names = get_param_names_from_constructor(self.__class__)
+        except SignatureError as e:
+            raise SignatureError("scikit-learn estimators should always "


This is still more or less duplicated with kernels. Should we share this?

This is still more or less duplicated with kernels. Should we share this?

I left that duplication to keep the error message of each class (scikit-learn estimators..., scikit-learn kernels...). I thought of 1) having the same scikit-learn estimators... message for both, or 2) passing a string argument to get_param_names_from_constructor to edit the message, but both felt awkward.

Though looking at it again, I noticed that set_params is duplicated too, so maybe Kernel could just inherit from BaseEstimator . Are there any historical reasons as to why it does not does so? By inheriting It could also benefit from the pickling and repr stuff.

Also, Kernels do not include any nested estimator-like parameters right? get_params is only shallow which seems enough, but set_params can handle nested parameters, why is this?

alegonz · 2019-06-26T13:34:32Z

Thanks. There's a lot going on, so I hope you can accept a slow review!

No worries! 👌

jnothman · 2019-06-27T10:24:41Z

Kernel does not have fit. We can have something else that is a common base class.

…

On Thu., 27 Jun. 2019, 12:03 am alegonz, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sklearn/base.py <#14175 (comment)> : > @@ -190,14 +175,17 @@ def get_params(self, deep=True): params : mapping of string to any Parameter names mapped to their values. """ - out = dict() - for key in self._get_param_names(): - value = getattr(self, key, None) - if deep and hasattr(value, 'get_params'): - deep_items = value.get_params().items() - out.update((key + '__' + k, val) for k, val in deep_items) - out[key] = value - return out + try: + param_names = get_param_names_from_constructor(self.__class__) + except SignatureError as e: + raise SignatureError("scikit-learn estimators should always " This is still more or less duplicated with kernels. Should we share this? I left that duplication to keep the error message of each class (scikit-learn estimators..., scikit-learn kernels...). I thought of 1) having the same scikit-learn estimators... message for both, or 2) passing a string argument to get_param_names_from_constructor to edit the message, but both felt awkward. Though looking at it again, I noticed that set_params is duplicated too, so maybe Kernel could just inherit from BaseEstimator . Are there any historical reasons as to why it does not does so? By inheriting It could also benefit from the pickling and repr stuff. Also, Kernels do not include any nested estimator-like parameters right? get_params is only shallow which seems enough, but set_params can handle nested parameters, why is this? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#14175?email_source=notifications&email_token=AAATH25ZAZJQZ2WZ7OMNSG3P4NZJXA5CNFSM4H27BY62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4WW4BQ#discussion_r297683358>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAATH234RZMOX3HRBUL5BJLP4NZJXANCNFSM4H27BY6Q> .

alegonz · 2019-06-28T00:33:05Z

Kernel does not have fit.

Oh, right. Forgot about that. I'm now realizing that even though Estimators are conceptually defined as having a fit method, BaseEstimator does not enforce this with, say, an @abstractmethod.

We can have something else that is a common base class.

Right, I'll see what I can do.

alegonz · 2019-06-30T05:02:01Z

Just updated the PR with further refactoring. This time I extracted get_params and set_params into a GetSetParamsMixin, and changed BaseEstimator and Kernel to inherit from it.

While it looks like the pickling and repr stuff in BaseEstimator could also be used in Kernel, I decided to keep it minimal for the moment and only extract get/set_params.

Note that BaseEstimator and Kernel still override get_params and set_params. This it to preserve the current docstrings and error messages (also, Kernel.get_params is overridden to be shallow). I tried to avoid this and have a single docstring and error message for both cases, but couldn't think of a word that encompasses both estimators and kernels without being too broad. (The methods in GetSetParamsMixin are worded generically.)

jnothman · 2019-07-09T13:39:18Z

sklearn/base.py

+        """
+        try:
+            return super().get_params(deep=deep)
+        except SignatureError as e:


I think I would be happier with super().__get_params(deep=deep, name='estimator') than the introduction of these new exception classes as sentinels.

Ok, I'll remove these exception classes and pass the name as an argument.

jnothman · 2019-07-09T13:39:32Z

sklearn/exceptions.py

+        self.param_name = param_name
+
+
+class SignatureError(RuntimeError):


I don't see why this should be public

You're right. I didn't think about that. Anyway, this will be gone as per the comment above.

alegonz · 2019-07-14T07:10:24Z

I removed the custom exception classes and rephrased the error messages.

I felt that, like the custom classes I had added, introducing a name='estimator' argument adds noise to the signature, and complicates things while adding little value. Both approaches are kinda overkill to just choose estimator or kernel in the error message. So I instead rephrased the error message of get_params and set_params to avoid these workarounds.

alegonz · 2019-07-14T07:13:15Z

sklearn/base.py

+                               "specify their parameters in the signature"
+                               " of their __init__ (no varargs)."
+                               " %s doesn't follow this convention."
+                               % self.__class__) from e


The from e will show the problematic signature in the error raised by get_param_names_from_constructor.

alegonz · 2019-07-14T07:15:02Z

sklearn/base.py

+                               " %s doesn't follow this convention."
+                               % self.__class__) from e
+
+    def set_params(self, **params):


Left to preserve the docstring.

I would just use the inherited method instead of overriding, but I left it as I wasn't sure if there are any rules regarding docstrings.

alegonz · 2019-07-14T07:15:36Z

sklearn/gaussian_process/kernels.py

-        for arg in args:
-            params[arg] = getattr(self, arg, None)
-        return params
+                               % self.__class__) from e

    def set_params(self, **params):


Same as above.

alegonz · 2019-08-09T15:09:12Z

@jnothman Just rebased onto latest master. The try except with the warning on AttributeError was also refactored :)

jnothman · 2019-08-12T07:19:51Z

Sorry I'm short on review time at the moment. Give us reasonable time to get to this and ping if you need to

…

alegonz · 2019-08-13T05:50:35Z

Sorry I'm short on review time at the moment. Give us reasonable time to get to this and ping if you need to
…

No worries 👍
Just updating with the latest changes so it doesn't go stale :)

…tion. This is meant to reuse it in gaussian_process.kernels.Kernel. Also added SignatureError to pass the signature and print the proper error message depending on the class.

…Kernel

…._make_selector

changed BaseEstimator and gaussian_process.kernels.Kernel to inherit from it. Also added InvalidParameterError to pass the parameter name and print the proper error message in set_params depending on the class.

…ams and set_params.

mglowacki100 · 2019-12-28T12:19:08Z

@alegonz
Hi, I'm dealing with the same issue and I checked your PR. Should I define get_params for my child classes or is it enough to inherit from your BaseEstimator? I'm asking because such drop-in replacement doesn't work for me with **kwargs (only parameters child class are returned).
Btw. now I use my own solution. Namely in child class I need only to define:

    def get_params(self, deep=True):
        """
        Get parameters.
        Based on: https://stackoverflow.com/questions/59248211
        """
        params = super().get_params(deep)
        cp = copy.copy(self)
        cp.__class__ = cp.__class__.__bases__[0]
        params.update(cp.__class__.get_params(cp, deep))
        return params

Subseqeuntly, all grandchildren have automagically proprer signature. However, there are a few disadvantages:

you can use abstract class with this trick
__bases__[0] makes assumptation about order in multiple inheritance
code looks tricky

I've looked at your solution, particularly clone function and it seems to be based on the same trick plus a lot of checks and MixIn due to kernels. Am I right or miss something?

alegonz · 2020-01-03T04:41:10Z

@mglowacki100

Hi there. Sorry for the late reply.

Should I define get_params for my child classes or is it enough to inherit from your BaseEstimator?

It is not enough to inherit from BaseEstimator. You would need to also override get_params to gather the parameters of its class and any super-class you might want to add explicitly. So for example you could do (copy-pasted from Issue #13555):

class MySubclass(SomeSKLearnEstimator):
    def __init__(self, myparam=42, **kwargs):
        # Just pass **kwargs.
        # No duplicated code.
        super().__init__(**kwargs)
        self.myparam = myparam
        
    def get_params(self, deep=True):
        # Gotta override get_params. But it shouldn't be hard.
        
        # params from parent
        parent_names = self._get_param_names(super())
        params = self._get_params_from(parent_names, deep=deep)
        
        # Add my params
        names = self._get_param_names(self.__class__)
        params.update(self._get_params_from(names, deep=deep))
        # or just simply
        # params['myparam'] = self.myparam
        
        return params


x = MySubclass(myparam=42, param1=2, param2='bar')
print(x.get_params())
# {'param1': 2, 'param2': 'bar', 'myparam': 42}

Currently _get_param_names(cls), which does the job of extracting the parameters from the __init__ signature, is a classmethod, making the above hard to achieve. So, the purpose of this PR is 1) to refactor that so things like the above becomes possible, and 2) remove some duplicated code in sklearn.gaussian_process.kernels.py.

AFAIK, without this PR one is left with three options:

Your approach, which I would not suggest for the reasons you stated. Also, I would add that that approach has another disadvantage of not being explicit on which super-classes its getting the parameters from.
Maybe you could also hack _get_param_names to unbind it from the class, so you can pass to it any class you like (see this comment).
Give up and duplicate the parameters on all the sub-classes :(

I've looked at your solution, particularly clone function and it seems to be based on the same trick plus a lot of checks and MixIn due to kernels. Am I right or miss something?

clone has been part of sklearn.base from long ago and this PR is not touching any of that.

adrinjalali · 2024-03-06T11:20:29Z

@alegonz this looks pretty good, would you be able to rebase on main and have a fresh start here?

alegonz · 2024-03-08T03:03:35Z

Hi @adrinjalali , sorry for the late reply.

Sure, I'd be happy to retake this PR. It's been a while since I opened this PR and the related issue so please give me some time to regain the context, re-study the code and address the rebase conflicts. Do you have any specific timeline for this PR?

adrinjalali · 2024-03-11T09:14:28Z

@alegonz not specific timeline. Just wanted to know if we should close this or give it to somebody else. But if you're happy to have another go, that'd be perfect.

jnothman reviewed Jun 25, 2019

View reviewed changes

jnothman reviewed Jul 9, 2019

View reviewed changes

alegonz force-pushed the refactor-get-param-names branch from e2de51e to 5a89f4d Compare July 14, 2019 05:20

alegonz changed the title ~~[WIP] Refactor BaseEstimator's _get_param_names and get_params~~ Refactor BaseEstimator's and Kernel's get_params and set_params Jul 14, 2019

alegonz commented Jul 14, 2019

View reviewed changes

alegonz force-pushed the refactor-get-param-names branch from 6fe90a6 to 9928673 Compare August 9, 2019 15:05

alegonz added 5 commits September 23, 2019 16:32

Moved _get_param_names out from BaseEstimator as a separate util func…

c15bcc6

…tion. This is meant to reuse it in gaussian_process.kernels.Kernel. Also added SignatureError to pass the signature and print the proper error message depending on the class.

Use new get_param_names_from_constructor in gaussian_process.kernels.…

a44f8c6

…Kernel

Use get_params instead of _get_param_names in GenericUnivariateSelect…

f26b10c

…._make_selector

Refactored get_params and set_params into a GetSetParamsMixin and

0ec0047

changed BaseEstimator and gaussian_process.kernels.Kernel to inherit from it. Also added InvalidParameterError to pass the parameter name and print the proper error message in set_params depending on the class.

Removed custom exception classes. Simplfied error messages in get_par…

8031fc0

…ams and set_params.

alegonz force-pushed the refactor-get-param-names branch from 9928673 to 8031fc0 Compare September 23, 2019 07:34

github-actions bot added module:feature_selection module:gaussian_process module:utils labels Mar 2, 2020

alegonz mentioned this pull request Jul 25, 2020

XGBoost is complaining about get_prams alegonz/baikal#36

Closed

Base automatically changed from master to main January 22, 2021 10:51

cmarmo added the Needs Decision Requires decision label Feb 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor BaseEstimator's and Kernel's get_params and set_params #14175

Refactor BaseEstimator's and Kernel's get_params and set_params #14175

alegonz commented Jun 24, 2019

jnothman left a comment

jnothman Jun 25, 2019

alegonz Jun 26, 2019

alegonz commented Jun 26, 2019

jnothman commented Jun 27, 2019 via email

alegonz commented Jun 28, 2019 •

edited

alegonz commented Jun 30, 2019

jnothman Jul 9, 2019

alegonz Jul 13, 2019

jnothman Jul 9, 2019

alegonz Jul 13, 2019

alegonz commented Jul 14, 2019

alegonz Jul 14, 2019

alegonz Jul 14, 2019

alegonz Aug 9, 2019

alegonz Jul 14, 2019 •

edited

alegonz commented Aug 9, 2019

jnothman commented Aug 12, 2019 via email

alegonz commented Aug 13, 2019

mglowacki100 commented Dec 28, 2019

alegonz commented Jan 3, 2020

adrinjalali commented Mar 6, 2024

alegonz commented Mar 8, 2024

adrinjalali commented Mar 11, 2024

		self.param_name = param_name


		class SignatureError(RuntimeError):

Refactor BaseEstimator's and Kernel's get_params and set_params #14175

Are you sure you want to change the base?

Refactor BaseEstimator's and Kernel's get_params and set_params #14175

Conversation

alegonz commented Jun 24, 2019

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alegonz commented Jun 26, 2019

jnothman commented Jun 27, 2019 via email

alegonz commented Jun 28, 2019 • edited

alegonz commented Jun 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alegonz commented Jul 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alegonz Jul 14, 2019 • edited

Choose a reason for hiding this comment

alegonz commented Aug 9, 2019

jnothman commented Aug 12, 2019 via email

alegonz commented Aug 13, 2019

mglowacki100 commented Dec 28, 2019

alegonz commented Jan 3, 2020

adrinjalali commented Mar 6, 2024

alegonz commented Mar 8, 2024

adrinjalali commented Mar 11, 2024

alegonz commented Jun 28, 2019 •

edited

alegonz Jul 14, 2019 •

edited