Fix loading latest XGBoost binary model. #144
Conversation
Codecov Report
@@ Coverage Diff @@
## master #144 +/- ##
============================================
- Coverage 40.77% 39.35% -1.43%
- Complexity 51 53 +2
============================================
Files 80 80
Lines 5913 5946 +33
Branches 37 37
============================================
- Hits 2411 2340 -71
- Misses 3476 3579 +103
- Partials 26 27 +1
Continue to review full report at Codecov.
|
|
@hcho3 Could you please take a look and test it yourself with dmlc/xgboost#5281 when time allows? I want to merge 2 PRs together, making sure that everything works as expected. |
|
@trivialfis The Treelite CI is using XGBoost 0.90 for integration tests. We need to test 1.0.0. Should I try the models you gave me earlier? |
|
@hcho3 For loading 0.90 the CI tests should be sufficient. Just testing treelite in combination with dmlc/xgboost#5281 . I tested it myself, but it would be great if you can test it too. |
|
Running the test I added in treelite should be sufficient. |
| @@ -390,6 +399,17 @@ inline treelite::Model ParseStream(dmlc::Stream* fi) { | |||
|
|
|||
| // set global bias | |||
| model.param.global_bias = static_cast<float>(mparam_.base_score); | |||
| std::vector<std::string> exponential_family { | |||
| "count:poisson", "reg:gamma", "reg:tweedie" | |||
There was a problem hiding this comment.
do we need to do something special for survival:cox? or assert that it is not supported?
There was a problem hiding this comment.
No, it is not necessary, since the label is not in log scale for survival:cox. For survival:cox, the convention is to use negative label to represent right-censored data.
There was a problem hiding this comment.
@hcho3 has better understanding of survival model than me.
|
|
||
| def test_logistic(self): | ||
| np.random.seed(0) | ||
| kRows = 16 |
There was a problem hiding this comment.
Is there a similar test for the exponential families? You could parametrize and reuse this with just a different randint limit for count:poisson probably.
There was a problem hiding this comment.
I ran the XGB integration test on my machine successfully with dmlc/xgboost#5281.
Great! |
|
@hcho3 Shall I merge the PR in XGBoost ? |
|
Yes |
For XGBoost >= 1.0.0, depends on dmlc/xgboost#5281 .