Make get_param_names a class method on single GPU estimators to match Scikit-learn closer

[dantegd]

Small difference between our estimators and Scikit-learn is that get_param_names are a classmethod in sklearn, and not in ours. This can make a few corner cases fail for using our estimators when Scikit-learn like estimators are expected. This PR fixes that.

Note: This will not include dask-based estimators for the time being since they depend on introspection at object creation time.

[divyegala]

Why were none of the Cython files changed?

[dantegd]

lol because I wrote a script to do this and only checked .py files instead of .pyx files too

[betatim]

Do you want to make them private at the same time? On the one hand, then we'd be 100% the same. On the other hand, the fact that they are private in scikit-learn makes me wonder if this matters (as they aren't part of the public API)?

[dantegd]

@betatim good point, I think matching as close as possible (i.e. entirely) is a good idea

[betatim]

Suggested change

To support cloning, estimators need to implement the function `_get_param_names()`. The returned value should be a list of strings of all estimator attributes that are necessary to duplicate the estimator. This method is used in `Base.get_params()` which will collect the collect the estimator param values from this list and pass this dictionary to a new estimator constructor. Therefore, all strings returned by `_get_param_names()` should be arguments in `__init__()` otherwise an invalid argument exception will be raised. Most estimators implement `_get_param_names()` similar to:

To support cloning, estimators need to implement the function `_get_param_names()`. The returned value should be a list of strings of all estimator attributes that are necessary to duplicate the estimator. This method is used in `Base.get_params()` which will collect the estimator param values from this list and pass this dictionary to a new estimator constructor. Therefore, all strings returned by `_get_param_names()` should be arguments in `__init__()` otherwise an invalid argument exception will be raised. Most estimators implement `_get_param_names()` similar to:

[betatim]

It is a big rename diff :D

Looks good from a quick check. One thing I noticed: some are classmethods (the majority) but some aren't. Oversight? If on purpose it is maybe worth adding a comment to the ones that aren't to help people from the future understand why they are different.

[dantegd]

@betatim the ones that haven't changed to be class methods are the dask-based estimators, currently they depend on some runtime behavior, I would suggest we do those on a follow up

[betatim]

Ok. Maybe they don't need a comment then.

Time to merge?

[dantegd]

/merge