Skip to content

TST Adjust learning rate to compare with LightGBM 3.X #21082

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

Merged
merged 3 commits into from
Sep 24, 2021

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Closes #18316 (comment)

What does this implement/fix? Explain your changes.

Adds the scaling factor for learning_rate when comparing with lightgbm and multiclass tasks.

Any other comments?

This PR will enable us to compare interaction constraints in the CI in PR #21020 (as stated in #18316 (comment)).

CC @lorentzenchr @ogrisel

Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

@thomasjpfan Thanks for tackling this one!

Comment on lines 232 to 235
# TODO: Used decimal=2 when seed=0. acc_sklearn is 0.02 higher than
# acc_lightgbm with seed=0
decimal = 1 if seed == 0 else 2
np.testing.assert_almost_equal(acc_lightgbm, acc_sklearn, decimal=decimal)
Copy link
Member

Choose a reason for hiding this comment

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

In principle, I'm against seed dependent test tolerances. Can this situation be improved?

thomasjpfan and others added 2 commits September 20, 2021 11:01
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Very nice fix, thanks!

@ogrisel ogrisel merged commit a2eb04d into scikit-learn:main Sep 24, 2021
@ogrisel
Copy link
Member

ogrisel commented Sep 24, 2021

We might want to backport this for 1.0.1.

@ogrisel ogrisel added this to the 1.0.1 milestone Sep 24, 2021
@glemaitre glemaitre mentioned this pull request Oct 23, 2021
10 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 23, 2021
…1082)


Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
glemaitre pushed a commit that referenced this pull request Oct 25, 2021
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
…1082)


Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests are failing with the new version of lightgbm (3.0.0)
3 participants