In cuml we don't have a systematic way of checking hyper-parameter values. Some estimators do a better job than others, the message formats are different across estimators, etc. I think we should adopt something that is similar to scikit-learn parameter validation framework.
With this we can increase coverage (in terms of number of parameters that are correctly validated) and reduce the amount of code in cuml (by not having to duplicate validation code).
This is about core cuml, in cuml.accel we don't need to change anything.
I'd build something that is based on scikit-learn's validation framework. We can't reuse it directly because it is private to scikit-learn and we probably want to do some things slightly differently. The way it works is by declaring a class level dict which maps parameter name to a list of constraints:
_parameter_constraints = {
"alpha": [Interval(Real, 0, None, closed="left"), "array-like"],
"fit_intercept": ["boolean"],
"max_iter": [Interval(Integral, 1, None, closed="left"), None],
"solver": [StrOptions({"eig", "svd", "cd"})],
"random_state": ["random_state"],
}Validation will be at fit time. In scikit-learn there is a dedicated decorator (_fit_context(prefer_skip_nested_validation=...)). Maybe we can fold it into mlfunc. We do need the prefer_skip_nested_validation parameter because sometimes you want to (e.g. meta estimators that are passed an estimator instance) and sometimes you don't want to do validation (e.g. an estimator that constructs estimators to do work for it).
We can add testing infra to make sure that all estimators are in the framework, that all parameters of each estimator have declared constraints.
I would stick with the approach of using this framework only to decide if a arguments value is valid or not. This means we will continue to need code that checks that two parameters that depend on each other contain compatible values.
I propose that we make one PR to introduce the machinery and convert one or two estimators to show it works. Then convert one estimator per PR where we add the declarative validation and remove now-no-longer-needed code.
Is deriving _get_param_names from _parameter_constraints the right way around, versus deriving parameter names from the __init__ signature the way scikit-learn does? This is a question because I am not sure I fully understand what _get_param_names does/why it is the way it is.
In cuml we don't have a systematic way of checking hyper-parameter values. Some estimators do a better job than others, the message formats are different across estimators, etc. I think we should adopt something that is similar to scikit-learn parameter validation framework.
With this we can increase coverage (in terms of number of parameters that are correctly validated) and reduce the amount of code in cuml (by not having to duplicate validation code).
This is about core cuml, in
cuml.accelwe don't need to change anything.I'd build something that is based on scikit-learn's validation framework. We can't reuse it directly because it is private to scikit-learn and we probably want to do some things slightly differently. The way it works is by declaring a class level
dictwhich maps parameter name to a list of constraints:Validation will be at
fittime. In scikit-learn there is a dedicated decorator (_fit_context(prefer_skip_nested_validation=...)). Maybe we can fold it intomlfunc. We do need theprefer_skip_nested_validationparameter because sometimes you want to (e.g. meta estimators that are passed an estimator instance) and sometimes you don't want to do validation (e.g. an estimator that constructs estimators to do work for it).We can add testing infra to make sure that all estimators are in the framework, that all parameters of each estimator have declared constraints.
I would stick with the approach of using this framework only to decide if a arguments value is valid or not. This means we will continue to need code that checks that two parameters that depend on each other contain compatible values.
I propose that we make one PR to introduce the machinery and convert one or two estimators to show it works. Then convert one estimator per PR where we add the declarative validation and remove now-no-longer-needed code.
Is deriving
_get_param_namesfrom_parameter_constraintsthe right way around, versus deriving parameter names from the__init__signature the way scikit-learn does? This is a question because I am not sure I fully understand what_get_param_namesdoes/why it is the way it is.