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

Jump to bottom

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

Merged
merged 3 commits into from Sep 24, 2021
Merged

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

merged 3 commits into from Sep 24, 2021

Conversation

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Sep 18, 2021

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
lorentzenchr
lorentzenchr approved these changes Sep 19, 2021
View changes
Copy link
Member

@lorentzenchr lorentzenchr left a comment

@thomasjpfan Thanks for tackling this one!

build_tools/azure/install.sh Outdated Show resolved Hide resolved
sklearn/ensemble/_hist_gradient_boosting/tests/test_compare_lightgbm.py Outdated
# 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

@lorentzenchr lorentzenchr Sep 19, 2021

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?

sklearn/ensemble/_hist_gradient_boosting/utils.pyx Show resolved Hide resolved
thomasjpfan and others added 2 commits Sep 20, 2021
@thomasjpfan @lorentzenchr
Apply suggestions from code review
112c23c 
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
@thomasjpfan
ENH Use atol to compare accuracy
43956be
ogrisel
ogrisel approved these changes Sep 24, 2021
View changes
Copy link
Member

@ogrisel ogrisel left a comment

Very nice fix, thanks!

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

@ogrisel 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
Merged
10 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this issue Oct 23, 2021
@thomasjpfan @lorentzenchr @glemaitre
TST Adjust learning rate to compare with LightGBM 3.X (scikit-learn#2…
9580e21 
…1082)


Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
glemaitre added a commit that referenced this issue Oct 25, 2021
@thomasjpfan @lorentzenchr @glemaitre
TST Adjust learning rate to compare with LightGBM 3.X (#21082)
da12f00 
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
samronsin added a commit to samronsin/scikit-learn that referenced this issue Nov 30, 2021
@thomasjpfan @lorentzenchr @samronsin
TST Adjust learning rate to compare with LightGBM 3.X (scikit-learn#2…
c61d4e5 
…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
Reviewers

@ogrisel ogrisel

@lorentzenchr lorentzenchr

Assignees
No one assigned
Labels
cython module:ensemble No Changelog Needed
Projects
None yet
Milestone
1.0.1
Linked issues

Successfully merging this pull request may close these issues.

Tests are failing with the new version of lightgbm (3.0.0)
3 participants
@thomasjpfan @ogrisel @lorentzenchr