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

[python] preserve None in _choose_param_value() #5289

Merged
merged 4 commits into from
Jun 19, 2022
Merged

[python] preserve None in _choose_param_value() #5289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5154daf
[python] preserve None in _choose_param_value()
jameslamb Jun 14, 2022
f29662a
Merge branch 'master' into fix/choose-param-value
jameslamb Jun 19, 2022
53876bd
Update python-package/lightgbm/basic.py
jameslamb Jun 19, 2022
6616078
Merge branch 'fix/choose-param-value' of github.com:microsoft/LightGB…
jameslamb Jun 19, 2022
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
30 changes: 18 additions & 12 deletions python-package/lightgbm/basic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,21 +408,27 @@ def _choose_param_value(main_param_name: str, params: Dict[str, Any], default_va
# avoid side effects on passed-in parameters
params = deepcopy(params)

# find a value, and remove other aliases with .pop()
# prefer the value of 'main_param_name' if it exists, otherwise search the aliases
found_value = None
aliases = _ConfigAliases.get(main_param_name) - {main_param_name}

# if main_param_name was provided, keep that value and remove all aliases
if main_param_name in params.keys():
found_value = params[main_param_name]
for param in aliases:
params.pop(param, None)
return params

for param in _ConfigAliases.get(main_param_name):
val = params.pop(param, None)
if found_value is None and val is not None:
found_value = val
# if main param name was not found, search for an alias
for param in aliases:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably for a different PR but shouldn't we try to match the behavior of the C++ side with respect to aliases and try to sort them using the same criteria than here?

LightGBM/include/LightGBM/config.h

Lines 1141 to 1144 in fc0c8fd

if (alias_set != tmp_map.end()) { // alias already set
// set priority by length & alphabetically to ensure reproducible behavior
if (alias_set->second.size() < pair.first.size() ||
(alias_set->second.size() == pair.first.size() && alias_set->second < pair.first)) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh wow, I actually did not know the C++ side did that!

Yes probably would be a good idea to consider that in a future PR. I could see "a different alias got chosen because the order of resolution changed" being a really subtle reproducibility problem that costs users some time, but would want to weigh that against the performance burden of adding more sorting size-checking code to the parameter-resolving stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@StrikerRUS @jmoralez I just created #5304 to track the work of making alias-resolution consistent and reproducible.

Please add any other details there that you think I might have missed.

if param in params.keys():
params[main_param_name] = params[param]
break

if found_value is not None:
params[main_param_name] = found_value
else:
params[main_param_name] = default_value
if main_param_name in params.keys():
for param in aliases:
params.pop(param, None)
return params
Comment on lines +425 to +428
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this the same as lines 414-417?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If main_param_name in params.keys(): is true in this block, it means "a value was provided via an alias". This needs to be here to clear out the other aliases.

That's different from lines 414-417, which is like "a value was provided via the main parameter name".

Copy link
Collaborator

@jmoralez jmoralez Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I see. Maybe after not finding the main_param_name we could do the following:

aliases_in_params = list(params.keys() & aliases)
if aliases_in_params:
    # if any alias was found, set the value of the first one
    params[main_param_name] = params[aliases_in_params[0]]
    # remove remaining aliases
    for alias in aliases_in_params[1:]:
        params.pop(alias, None)
    return params

I'm approving since this is just a suggestion. Thanks for the explanation!


# neither of main_param_name, aliases were found
params[main_param_name] = default_value

return params

Expand Down
36 changes: 36 additions & 0 deletions tests/python_package_test/test_basic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,42 @@ def test_choose_param_value():
assert original_params == expected_params


def test_choose_param_value_preserves_nones():

# preserves None found for main param and still removes aliases
params = lgb.basic._choose_param_value(
main_param_name="num_threads",
params={
"num_threads": None,
"n_jobs": 4,
"objective": "regression"
},
default_value=2
)
assert params == {"num_threads": None, "objective": "regression"}

# correctly chooses value when only an alias is provided
params = lgb.basic._choose_param_value(
main_param_name="num_threads",
params={
"n_jobs": None,
"objective": "regression"
},
default_value=2
)
assert params == {"num_threads": None, "objective": "regression"}

# adds None if that's given as the default and param not found
params = lgb.basic._choose_param_value(
main_param_name="min_data_in_leaf",
params={
"objective": "regression"
},
default_value=None
)
assert params == {"objective": "regression", "min_data_in_leaf": None}


@pytest.mark.parametrize("objective_alias", lgb.basic._ConfigAliases.get("objective"))
def test_choose_param_value_objective(objective_alias):
# If callable is found in objective
Expand Down