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.

Sign up for GitHub

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] add tests on warnings, fix incorrect variable in log #3865

Merged
merged 2 commits into from
Jan 27, 2021
Merged

[dask] add tests on warnings, fix incorrect variable in log #3865

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da5fe0d
[dask] add tests on warnings, fix incorrect variable in log
jameslamb Jan 26, 2021
07eefcb
Update tests/python_package_test/test_dask.py
jameslamb Jan 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion python-package/lightgbm/dask.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ def _train(client, data, label, params, model_factory, sample_weight=None, group
'voting_parallel'
}
if params["tree_learner"] not in allowed_tree_learners:
_log_warning('Parameter tree_learner set to %s, which is not allowed. Using "data" as default' % tree_learner)
_log_warning('Parameter tree_learner set to %s, which is not allowed. Using "data" as default' % params['tree_learner'])
params['tree_learner'] = 'data'

if params['tree_learner'] not in {'data', 'data_parallel'}:
Expand Down
34 changes: 34 additions & 0 deletions tests/python_package_test/test_dask.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,40 @@ def test_find_open_port_works():
assert new_port == 12402


def test_warns_and_continues_on_unrecognized_tree_learner(client):
X = da.random.random((1e3, 10))
y = da.random.random((1e3, 1))
dask_regressor = lgb.DaskLGBMRegressor(
time_out=5,
local_listen_port=1234,
tree_learner='some-nonsense-value',
n_estimators=1,
num_leaves=2
)
with pytest.warns(UserWarning, match='Parameter tree_learner set to some-nonsense-value'):
dask_regressor = dask_regressor.fit(X, y, client=client)

assert dask_regressor.fitted_


def test_warns_but_makes_no_changes_for_feature_or_voting_parallel(client):
X = da.random.random((1e3, 10))
y = da.random.random((1e3, 1))
for tree_learner in ['feature_parallel', 'voting']:
dask_regressor = lgb.DaskLGBMRegressor(
time_out=5,
local_listen_port=1234,
tree_learner=tree_learner,
n_estimators=1,
num_leaves=2
)
with pytest.warns(UserWarning, match='Support for tree_learner %s in lightgbm' % tree_learner):
dask_regressor = dask_regressor.fit(X, y, client=client)

assert dask_regressor.fitted_
assert dask_regressor.get_params()['tree_learner'] == tree_learner
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wonder: is it possible to access params via attributes like it can be done in sklearn-wrapper?

dask_regressor.tree_learner

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe! I haven't tested. I didn't actually know the sklearn interface supported that.

But either way, I have a preference for get_params() here because it also gives us some confidence that this is working:

LightGBM/python-package/lightgbm/dask.py

Lines 419 to 420 in 59153b2

self.set_params(**model.get_params())
self._copy_extra_params(model, self)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly set_params() should set attributes btw

LightGBM/python-package/lightgbm/sklearn.py

Lines 358 to 376 in b4b1b75

def set_params(self, **params):
"""Set the parameters of this estimator.
Parameters
----------
**params
Parameter names with their new values.
Returns
-------
self : object
Returns self.
"""
for key, value in params.items():
setattr(self, key, value)
if hasattr(self, '_' + key):
setattr(self, '_' + key, value)
self._other_params[key] = value
return self



@gen_cluster(client=True, timeout=None)
def test_errors(c, s, a, b):
def f(part):
Expand Down