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

[python-package] respect parameter aliases for network params #3813

Merged
merged 19 commits into from
Jan 26, 2021

Conversation

jameslamb
Copy link
Collaborator

Based on #3789 (review). This PR proposes some changes in the Booster constructor to respect aliases for network parameters.

This PR also has the following small changes

  • replaces mistaken parameter name "listen_time_out" with "time_out"
  • adds a rule to .gitignore to ignore .pytest_cache/ directories (I found these were created but not ignored while I was testing)

Notes for Reviewers

@jameslamb jameslamb requested a review from StrikerRUS January 25, 2021 06:07
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thanks for improving alias handling! Please check my comments:

python-package/lightgbm/basic.py Show resolved Hide resolved
@@ -352,6 +354,46 @@ def get(cls, *args):
return ret


def _choose_param_value(main_param_name: str, params: Dict[str, Any], default_value: Any) -> Dict[str, Any]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

TBH, I don't like _choose_param_value name very much, but don't have alternatives to suggest. Maybe you have?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like this name very much and think it's descriptive of what is happening. Given a dictionary of parameters, it chooses a single value to use for a single parameter.

python-package/lightgbm/basic.py Outdated Show resolved Hide resolved
local_listen_port=params["local_listen_port"],
listen_time_out=params.get("listen_time_out", 120),
num_machines=params["num_machines"]
)
break
Copy link
Collaborator

@StrikerRUS StrikerRUS Jan 25, 2021

Choose a reason for hiding this comment

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

I think we should remove all aliases of machines from params. This break prevents doing this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, I proposed something for this in 63387db

I think the cleanest way to do it is to use _choose_param_value() to get a single value for machines. Then we don't need a separate for loop with a break.

python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
Comment on lines +371 to +376
expected_params = {
"local_listen_port": 1234,
"port": 2222,
"metric": "auc",
"num_trees": 81
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think expected_params can be gotten by copying original_params at the very beginning of the test to not re-type every value again here.

Suggested change
expected_params = {
"local_listen_port": 1234,
"port": 2222,
"metric": "auc",
"num_trees": 81
}
expected_params = copy(original_params)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

copy() performs a shallow clone, so some modifications to original_params can still impact the copy. https://docs.python.org/3/library/copy.html#copy.copy

I think it's preferable for this test to re-type the literal values, so that the test doesn't get a false positive because of subtle mistakes where we assumed something was a copy but actually was a reference

@StrikerRUS
Copy link
Collaborator

@jameslamb
Could you please resolve conflicts?

@jameslamb
Copy link
Collaborator Author

@jameslamb
Could you please resolve conflicts?

done

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

@jameslamb Thanks a lot for cleaning up aliases handling logic!
LGTM except one typo that you've already mentioned earlier

replaces mistaken parameter name "listen_time_out" with "time_out"

python-package/lightgbm/basic.py Outdated Show resolved Hide resolved
python-package/lightgbm/dask.py Outdated Show resolved Hide resolved
tests/python_package_test/test_basic.py Outdated Show resolved Hide resolved
@jameslamb jameslamb merged commit 9f70e96 into microsoft:master Jan 26, 2021
@jameslamb jameslamb deleted the fix/param-aliases branch January 26, 2021 16:00
@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.

2 participants