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

xgboost: add clipping of loss values to the float32 limits #35

Merged
merged 2 commits into from
Jan 13, 2023

Conversation

AldoGl
Copy link
Contributor

@AldoGl AldoGl commented Jan 13, 2023

Proposed changes

Internally the XGBoost routine operates a conversion to float32, and this fails if the absolute values of the loss are exceedingly large. This commit adds a simple clipping operation before the loss values are passed to the XGBoost package.

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

@AldoGl AldoGl force-pushed the fix-xgboost-float-problem branch from 0e6ecfe to d4f1a45 Compare January 13, 2023 13:12
@AldoGl AldoGl requested a review from muxator January 13, 2023 13:47
@AldoGl AldoGl force-pushed the fix-xgboost-float-problem branch from d4f1a45 to 8152d44 Compare January 13, 2023 13:49
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2023

Codecov Report

Merging #35 (37ab3e7) into main (03da1df) will decrease coverage by 0.03%.
The diff coverage is 94.11%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
- Coverage   97.21%   97.17%   -0.04%     
==========================================
  Files          29       29              
  Lines        1435     1452      +17     
==========================================
+ Hits         1395     1411      +16     
- Misses         40       41       +1     
Impacted Files Coverage Δ
black_it/samplers/xgboost.py 98.21% <94.11%> (-1.79%) ⬇️

@AldoGl AldoGl force-pushed the fix-xgboost-float-problem branch 2 times, most recently from 37ab3e7 to d334ac1 Compare January 13, 2023 14:13
@muxator muxator force-pushed the fix-xgboost-float-problem branch from d334ac1 to e73efe0 Compare January 13, 2023 14:16
@AldoGl AldoGl force-pushed the fix-xgboost-float-problem branch from e73efe0 to d334ac1 Compare January 13, 2023 14:21
muxator and others added 2 commits January 13, 2023 15:29
Internally the XGBoost routine operates a conversion to float32, and this fails
if the absolute values of the loss are exceedingly large. This commit adds a
simple clipping operation before the loss values are passed to the XGBoost
package.
@AldoGl AldoGl force-pushed the fix-xgboost-float-problem branch from d334ac1 to 661d981 Compare January 13, 2023 14:33
@muxator muxator force-pushed the fix-xgboost-float-problem branch 2 times, most recently from 4bb6668 to d6a67b8 Compare January 13, 2023 14:41
@muxator
Copy link

muxator commented Jan 13, 2023

  • split the PR in two commits. The first one triggers the problem, the second one shows the fix. The final version is the same as the one submitted by @AldoGl.
  • some lint fixes

@AldoGl
Copy link
Contributor Author

AldoGl commented Jan 13, 2023

  • split the PR in two commits. The first one triggers the problem, the second one shows the fix. The final version is the same as the one submitted by @AldoGl.
  • some lint fixes

Thank you for your help @muxator, merging now!

@AldoGl AldoGl merged commit 46f6088 into main Jan 13, 2023
@AldoGl AldoGl deleted the fix-xgboost-float-problem branch January 13, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants