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

adjusted bagging_freq parameter description #5698

Merged
merged 3 commits into from
Feb 13, 2023
Merged

adjusted bagging_freq parameter description #5698

merged 3 commits into from
Feb 13, 2023

Conversation

moziada
Copy link
Contributor

@moziada moziada commented Feb 3, 2023

No description provided.

Copy link
Collaborator

@jameslamb jameslamb 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 your interest in LightGBM and for taking the time to modify the documentation.

I'm ok with this change to the docs. However, we have a slightly different process for modifying the Parameters documentation page.

Please modify this:

// desc = **Note**: to enable bagging, ``bagging_fraction`` should be set to value smaller than ``1.0`` as well

Then run this from the root of the repo

python helpers/parameter_generator.py

and commit the resulting changes.


In the future, I strongly recommend making pull requests from a new branch on your fork, instead of the target repo's default branch (master, in LightGBM's case). This project uses squash-merging, so master on your fork and master here in the main repository are going to have different histories now that you've created a pull request from master in your fork.

Once this pull request is merged, I recommend deleting your fork of LightGBM and then re-forking if you want to make further contributions here.

@moziada
Copy link
Contributor Author

moziada commented Feb 7, 2023

Hi James,
After editing config.h and running python helpers/parameter_generator.py command as you mentioned, git showed that config.h is only the file that has changes.
And sure for future contributions I will make a pull requests from a new branch rather than master

@moziada
Copy link
Contributor Author

moziada commented Feb 7, 2023

@moziada please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

agree

@@ -333,7 +333,7 @@ struct Config {
// alias = subsample_freq
// desc = frequency for bagging
// desc = ``0`` means disable bagging; ``k`` means perform bagging at every ``k`` iteration. Every ``k``-th iteration, LightGBM will randomly select ``bagging_fraction * 100 %`` of the data to use for the next ``k`` iterations
// desc = **Note**: to enable bagging, ``bagging_fraction`` should be set to value smaller than ``1.0`` as well
// desc = **Note**: to enable bagging, ``bagging_fraction`` should be set to a value between ``0.0`` and ``1.0``
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we maybe change the wording a bit? The original intent is to clarify that bagging_fraction=1.0 disables bagging. Between usually means inclusive on both sides, but this param should be greater than 0 and less than 1 to enable it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we maybe change the wording a bit? The original intent is to clarify that bagging_fraction=1.0 disables bagging. Between usually means inclusive on both sides, but this param should be greater than 0 and less than 1 to enable it.

Clarification is made

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Can you please also sign the CLA? You need to post a single comment like the examples that were shown in #5698 (comment), for example

@microsoft-github-policy-service agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@moziada
Copy link
Contributor Author

moziada commented Feb 8, 2023

@microsoft-github-policy-service agree

@jameslamb
Copy link
Collaborator

After editing config.h and running python helpers/parameter_generator.py command as you mentioned, git showed that config.h is only the file that has changes

Thanks for doing that! That's because you'd already manually changed docs/Parameters.rst. If you had changed config.h then run that Python script, it would have altered docs/Parameters.rst.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

works for me, thanks!

@jameslamb
Copy link
Collaborator

Looks like all of @jmoralez 's suggestions have been addressed, so I'm going to merge this.

Thanks for taking the time to contribute 👋🏻

@jameslamb jameslamb merged commit 771bad8 into microsoft:master Feb 13, 2023
@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 16, 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

3 participants