Support init arguments in MNMG LogisticRegression #5519
Conversation
lijinf2
added
improvement
| def __init__(self, *, handle=None): | ||
| super().__init__(handle=handle) | ||
| def __init__( | ||
| self, |
There was a problem hiding this comment.
In other dask estimators, we pass **kwargs through to the super class so that we don't need to specify the defaults twice in the cython layer. Is there a reason we can't do that here? As an example.
There was a problem hiding this comment.
The purpose was to restrict the set of arguments to those that support L-BFGS. The added class-level documentation should serve the purpose so I revised to **kwargs to avoid duplicates. Thanks for the suggestion!
| @@ -31,7 +32,8 @@ | |||
| np = cpu_only_import("numpy") | |||
|
|
|||
|
|
|||
| class LogisticRegression(BaseEstimator, SyncFitMixinLinearModel): | |||
| # class LogisticRegression(BaseEstimator, SyncFitMixinLinearModel): | |||
| class LogisticRegression(LinearRegression): | |||
| def __init__(self, *, client=None, verbose=False, **kwargs): | |||
There was a problem hiding this comment.
I do think there's a case to be made for specifying defaults in the constructor at this layer, and they should end up getting propagated to the cython class. Another thing we missed in the review of the initial Dask estimator is that this should be documented at the class level. Can you add those docs in this PR too? I think most of the docs can likely be copied from the single gpu LogisticRegression
There was a problem hiding this comment.
Good suggestion. I have revised the init function and added class-level documentation.
|
/merge |
The init arguments are for LBFGS (the only algorithm in the current MNMG LogisticRegression).
The key code changes should be a few lines after PR 5516 for predict gets merged. Key code changes can be reviewed from here