search

rapidsai / cuml

cuML - RAPIDS Machine Learning Library
https://docs.rapids.ai/api/cuml/stable/
Apache License 2.0
4.25k stars 534 forks source link

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

Closed dantegd closed 1 week ago

dantegd commented 1 month ago

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.

dantegd commented 1 month ago

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

betatim commented 1 month ago

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 commented 1 month ago

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

betatim commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

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

Time to merge?

dantegd commented 1 week ago

/merge