-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fixed light gbm update #7431
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
Fixed light gbm update #7431
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the LightGBM interface to accommodate changes in the latest LightGBM version.
- The API signature in WrappedLightGbmInterface.cs was updated by replacing a single parameter (numTotalRow) with two separate parameters (numTotalLocalRow and numTotalDistributedRow).
- The API call in WrappedLightGbmDataset.cs was updated to pass a hardcoded value for the new numTotalDistributedRow parameter.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/Microsoft.ML.LightGbm/WrappedLightGbmInterface.cs | Updated method signature to split numTotalRow into two parameters. |
src/Microsoft.ML.LightGbm/WrappedLightGbmDataset.cs | Updated method call to include the new parameter with a 0 literal. |
Files not reviewed (1)
- eng/Versions.props: Language not supported
Comments suppressed due to low confidence (2)
src/Microsoft.ML.LightGbm/WrappedLightGbmInterface.cs:91
- [nitpick] The new parameter 'numTotalDistributedRow' might benefit from additional inline documentation to clarify its intended use compared to 'numTotalLocalRow'.
int numTotalDistributedRow,
src/Microsoft.ML.LightGbm/WrappedLightGbmDataset.cs:70
- [nitpick] Consider replacing the hardcoded literal '0' with a named constant or adding a comment to explain its significance for improved readability.
(IntPtr)ptrValues, (IntPtr)ptrIndices, numCol, sampleNonZeroCntPerColumn, numSampleRow, numTotalRow, 0,
Can LightGBM 4.* load model created by LightGBM 3.*? |
I saw mention of this here microsoft/LightGBM#2228 (comment) it seems that what we are doing should be supported. I was hoping to find something about it in the docs though. @michaelgsharp - oddly it seems like TensorFlow tests are failing on windows jobs in this PR with higher accuracy. I'm not sure what to think about that - but it's pretty consistent across multiple machines. 🤔 |
@ericstj just looked into the failing test. It looks like in that case TensorFlow is used only to featurize the data. That data is then fed into a LightGBM pipeline.. so thats the corrolation between them. |
I'll get the test updated with the new values since it does seem to be directly related to lightGBM in this specific case. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7431 +/- ##
=======================================
Coverage 68.99% 68.99%
=======================================
Files 1482 1482
Lines 273876 273876
Branches 28250 28250
=======================================
+ Hits 188966 188968 +2
+ Misses 77529 77523 -6
- Partials 7381 7385 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
So it looks like this LightGBM update improved the accuracy of that test. Bonus! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Failures are known due to file sharing between tests @LittleLittleCloud |
Fixes #7320.
Fixes #7045.
Updated light GBM to the latest version.