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

[docs] fixing max_depth param description #1879

Closed
wants to merge 1 commit into from
Closed

[docs] fixing max_depth param description #1879

wants to merge 1 commit into from

Conversation

KOLANICH
Copy link

@KOLANICH KOLANICH commented Nov 25, 2018

@StrikerRUS StrikerRUS changed the title Fixing the docs [docs] fixing max_depth param description Nov 30, 2018
@StrikerRUS
Copy link
Collaborator

@KOLANICH Thanks for your contribution!

Please follow this procedure to update parameter description.

@KOLANICH
Copy link
Author

BTW, why not to do it on CI automatically?

@StrikerRUS
Copy link
Collaborator

Because after generating anything on CI side it should be pushed back to GitHub.

@KOLANICH
Copy link
Author

Because after generating anything on CI side it should be pushed back to GitHub.

  1. What's the problem with it? Azure Pipelines support secret variables.
  2. If the docs are generated from the source, why not to remove them completely from the repo?

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented Nov 30, 2018

  1. What's the problem with it? Azure Pipelines support secret variables.

It's still insecure. Also, It produces additional mess in the repo. Imagine: every commit is duplicated by the CI.

  1. If the docs are generated from the source,

Only params description is generated from the source.

Laurae2 added a commit that referenced this pull request May 8, 2019
@Laurae2 Laurae2 mentioned this pull request May 8, 2019
@StrikerRUS
Copy link
Collaborator

@KOLANICH Remembering your position about the CLA #1873 (comment), we are taking over your PRs, if you don't mind.

so feel free to close the PR and add the links and everything else you feel needed yourself from your account and your name, I don't object, …

Can you please clarify your comment?
#1879 (comment)

https://github.com/Microsoft/LightGBM/blob/59f10453dc4a3841339702524124063fa5db6870/src/treelearner/serial_tree_learner.cpp#L351
BTW, should it be unsigned?

@KOLANICH
Copy link
Author

I just wonder why a signed int is used for a parameter which semantics is to be a number >= 0. Shouldn't an unsigned int be used for that?

@StrikerRUS
Copy link
Collaborator

Ah, got it, thanks!
But users can pass a negative value, e.g. -1 to indicate "no limit".

@KOLANICH
Copy link
Author

But users can pass a negative value, e.g. -1 to indicate "no limit".

But 0 is also "no limit".

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented May 14, 2019

You are right!
It's possible to save all semantics with unsigned value. However, this change will be not backward compatible.

BTW, at present, all params are signed values, even num_boost_round and other which should be strictly greater zero.

StrikerRUS pushed a commit that referenced this pull request May 15, 2019
* PR #1879

* Update docs with parameter_generator.py

* Update wrapper doc for sklearn
@StrikerRUS
Copy link
Collaborator

Proposed changes were made in #2155.

@StrikerRUS StrikerRUS closed this May 15, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
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