Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dask] remove extra 'client' kwarg in DaskLGBMRegressor #3906

Merged
merged 1 commit into from
Feb 3, 2021

Conversation

jameslamb
Copy link
Collaborator

Looks like I missed removing the client argument in DaskLGBMRegressor in #3883 , sorry!

I think in the future, we might be able to use mypy to catch API differences between the Dask package and its scikit-learn equivalents. For now, this PR removes that argument.

@StrikerRUS
Copy link
Collaborator

I think in the future, we might be able to use mypy to catch API differences between the Dask package and its scikit-learn equivalents. For now, this PR removes that argument.

I believe this approach you used for init with inspect is more reliable.

def test_dask_classes_and_sklearn_equivalents_have_identical_constructors_except_client_arg(classes):

@jameslamb
Copy link
Collaborator Author

I believe this approach you used for init with inspect is more reliable.

It is, but at the expense of a bit more maintenance burden. Because the test will have to explicitly articulate which keyword args are currently in the signature, and which are passed through with **kwargs**.

Anyway, for now the inspect() thing is probably good to do. I'll open a PR in a bit with tests like that for fit() and predict().

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants