Repository: BayesWitnesses/m2cgen (language: rust)

Issue:
Code generated for XGBoost models returns invalid scores when tree_method is set to "hist"
I have trained xgboost models in Python and am using the CLI interface to convert the serialized models to pure python.  However, when I use the pure python, the results differ from the predictions using the model directly.

Python 3.7
xgboost 0.90

My model has a large number of parameters (somewhat over 500).
Here are predicted class probabilities from the original model:
![image](https://user-images.githubusercontent.com/30508983/74898682-d3dda580-5357-11ea-95ea-cab6102615fa.png)

Here are the same predicted probabilities using the generated python code via m2cgen:
![image](https://user-images.githubusercontent.com/30508983/74898713-e9eb6600-5357-11ea-8607-bfb1cfbe82fc.png)

We can see that the results are similar but not the same.  The result is a significant number of cases that are moved into different classes between the two sets of predictions.

I have also tested this with binary classification models and have the same issues.  


Discussion / hints:
Hey @eafpres ! Thank you so much for reporting this. This is indeed quite bad. 

It'd be very helpful if you can provide some steps to reproduce the issue. Perhaps model's hyperparameters, or some sort of a synthetic dataset on which the problem reproduces.

**UPD** If you can confirm whether the same problem occurs in other languages (eg Java) that'd be just fantastic.
@izeigerman I will try to create a repro for you!  Thank you for looking into it.  FYI, to be clear it isn't just multiclass, the binary cases seem to produce the same issue (for me).
Something is clearly wrong there. Eg. row 13 - probabilities don't even add up to 1.0 in m2cgen's predictions, which doesn't make sense.
@eafpres Got it! May I please also ask you to share the m2cgen version that you're using (`m2cgen -v`)?  
Hi--version:

m2cgen 0.6.0

note: installed using pip on 2002-02-19
Excellent, thank you. Another thing you may try is to downgrade to version 0.5.0. We had some significant changes in XGBoost-related code in 0.6.0 which may or may not have caused this behavior. 
Hello @eafpres !

Brief question: are you using DART in your XGBoost model?
@StrikerRUS -- I am not using DART, here is my configuration:
```
        model = xgb.XGBClassifier(colsample_bytree = colsample_bytree,
                                  gamma = gamma,
                                  learning_rate = learning_rate,
                                  max_depth = max_depth,
                                  min_child_weight = min_child_weight,
                                  n_estimators = n_iters,
                                  reg_alpha = reg_alpha,
                                  reg_lambda = reg_lambda,
                                  subsample = subsample,
                                  tree_method = 'hist',
                                  objective = objective,
                                  booster = 'gbtree',
                                  eval_metric = eval_metric,
                                  num_class = n_class)
```
I've got a reproduce. `tree_method='hist'` seems to be causing this:
```
expected=[0.7084146  0.14699726 0.14458819], actual=[0.7084145564550915, 0.14699725651673898, 0.14458818702816942]
expected=[0.14497484 0.14953361 0.7054915 ], actual=[0.14497486252688704, 0.14953362429410053, 0.7054915131790125]
expected=[0.14497484 0.14953361 0.7054915 ], actual=[0.14497486252688704, 0.14953362429410053, 0.7054915131790125]
expected=[0.7084146  0.14699726 0.14458819], actual=[0.7084145564550915, 0.14699725651673898, 0.14458818702816942]
expected=[0.14489177 0.7067961  0.14831214], actual=[0.14489178040434617, 0.7067960730189182, 0.14831214657673555]
expected=[0.14527214 0.7086516  0.14607628], actual=[0.14527215259960496, 0.7086515652569474, 0.14607628214344756]
expected=[0.14497484 0.14953361 0.7054915 ], actual=[0.14497486252688704, 0.14953362429410053, 0.7054915131790125]
expected=[0.14489177 0.7067961  0.14831214], actual=[0.14489178040434617, 0.7067960730189182, 0.14831214657673555]
expected=[0.14497484 0.14953361 0.7054915 ], actual=[0.14497486252688704, 0.14953362429410053, 0.7054915131790125]
expected=[0.14497484 0.14953361 0.7054915 ], actual=[0.14497486252688704, 0.14953362429410053, 0.7054915131790125]
expected=[0.7084146  0.14699726 0.14458819], actual=[0.7084145564550915, 0.14699725651673898, 0.14458818702816942]
expected=[0.7084146  0.14699726 0.14458819], actual=[0.7084145564550915, 0.14699725651673898, 0.14458818702816942]
expected=[0.14527214 0.7086516  0.14607628], actual=[0.14527215259960496, 0.7086515652569474, 0.14607628214344756]
expected=[0.15254477 0.66059124 0.18686394], actual=[0.14527215259960496, 0.7086515652569474, 0.14607628214344756]
```
(Last row)
@eafpres for the sake of this investigation, can you please try without `tree_method='hist'` to see whether the problem persists?
@izeigerman -- I will test it and report back.  Thanks for the amazing support.
Hi @izeigerman -- I changed my configuration to use tree_method = 'exact' and the results are now exactly identical from the two models.  This was for the multi-class case.  I'll also be testing the binary case and confirm that.
@eafpres Got it, thank you so much for helping to triage the problem. We'll then be looking into why it misbehaves when the `tree_method` is set to `hist`.
@eafpres @izeigerman 
Can you guys please post a repro for this error here, because I cannot spot the issue by simply adding `tree_method='hist'` here
https://github.com/BayesWitnesses/m2cgen/blob/d058b8f64a86a075d4ee10998836f1f7b056e04b/tests/e2e/test_e2e.py#L93-L94
@StrikerRUS as far as I remember this is exactly how I reproduced it. Have you tried the `LARGE` version of this test? Or perhaps increase the sample size, because some rows still match with this tree method.
@izeigerman Hmm, interesting... Thank you for the hint! I'll continue investigation.
I can get you a repro in the next couple of days.

On Sat, Mar 21, 2020 at 7:34 AM Nikita Titov <notifications@github.com>
wrote:

> @izeigerman <https://github.com/izeigerman> Hmm, interesting... Thank you
> for the hint! I'll continue investigation.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/BayesWitnesses/m2cgen/issues/168#issuecomment-602044176>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHIYPNYZT27YIVSWUSYH6JTRIS67DANCNFSM4KYGNTOQ>
> .
>
-- 
Best regards,
Blaine Bateman
Founder & Chief Data Officer, EAF LLC

Inline image


Fix patch:
diff --git a/m2cgen/assemblers/boosting.py b/m2cgen/assemblers/boosting.py
index e327b774..714da1b6 100644
--- a/m2cgen/assemblers/boosting.py
+++ b/m2cgen/assemblers/boosting.py
@@ -134,7 +134,7 @@ def _assemble_tree(self, tree):
         if "leaf" in tree:
             return ast.NumVal(tree["leaf"])
 
-        threshold = ast.NumVal(tree["split_condition"])
+        threshold = ast.NumVal(tree["split_condition"], dtype=np.float32)
         split = tree["split"]
         feature_idx = self._feature_name_to_idx.get(split, split)
         feature_ref = ast.FeatureRef(feature_idx)
diff --git a/m2cgen/ast.py b/m2cgen/ast.py
index 9b49936b..f4195af7 100644
--- a/m2cgen/ast.py
+++ b/m2cgen/ast.py
@@ -30,7 +30,9 @@ class NumExpr(Expr):
 
 
 class NumVal(NumExpr):
-    def __init__(self, value):
+    def __init__(self, value, dtype=None):
+        if dtype:
+            value = dtype(value)
         self.value = value
 
     def __str__(self):


Test patch:
diff --git a/tests/e2e/test_e2e.py b/tests/e2e/test_e2e.py
index b92185f8..8b6e8baa 100644
--- a/tests/e2e/test_e2e.py
+++ b/tests/e2e/test_e2e.py
@@ -35,50 +35,51 @@
 
 
 # Set of helper functions to make parametrization less verbose.
-def regression(model):
+def regression(model, test_fraction=0.02):
     return (
         model,
-        utils.get_regression_model_trainer(),
+        utils.get_regression_model_trainer(test_fraction),
         REGRESSION,
     )
 
 
-def classification(model):
+def classification(model, test_fraction=0.02):
     return (
         model,
-        utils.get_classification_model_trainer(),
+        utils.get_classification_model_trainer(test_fraction),
         CLASSIFICATION,
     )
 
 
-def classification_binary(model):
+def classification_binary(model, test_fraction=0.02):
     return (
         model,
-        utils.get_binary_classification_model_trainer(),
+        utils.get_binary_classification_model_trainer(test_fraction),
         CLASSIFICATION,
     )
 
 
-def regression_random(model):
+def regression_random(model, test_fraction=0.02):
     return (
         model,
-        utils.get_regression_random_data_model_trainer(0.01),
+        utils.get_regression_random_data_model_trainer(test_fraction),
         REGRESSION,
     )
 
 
-def classification_random(model):
+def classification_random(model, test_fraction=0.02):
     return (
         model,
-        utils.get_classification_random_data_model_trainer(0.01),
+        utils.get_classification_random_data_model_trainer(test_fraction),
         CLASSIFICATION,
     )
 
 
-def classification_binary_random(model):
+def classification_binary_random(model, test_fraction=0.02):
     return (
         model,
-        utils.get_classification_binary_random_data_model_trainer(0.01),
+        utils.get_classification_binary_random_data_model_trainer(
+            test_fraction),
         CLASSIFICATION,
     )
 
@@ -92,6 +93,8 @@ def classification_binary_random(model):
 FOREST_PARAMS = dict(n_estimators=10, random_state=RANDOM_SEED)
 XGBOOST_PARAMS = dict(base_score=0.6, n_estimators=10,
                       random_state=RANDOM_SEED)
+XGBOOST_HIST_PARAMS = dict(base_score=0.6, n_estimators=10,
+                           tree_method="hist", random_state=RANDOM_SEED)
 XGBOOST_PARAMS_LINEAR = dict(base_score=0.6, n_estimators=10,
                              feature_selector="shuffle", booster="gblinear",
                              random_state=RANDOM_SEED)
@@ -170,6 +173,14 @@ def classification_binary_random(model):
         classification(xgboost.XGBClassifier(**XGBOOST_PARAMS)),
         classification_binary(xgboost.XGBClassifier(**XGBOOST_PARAMS)),
 
+        # XGBoost (tree method "hist")
+        regression(xgboost.XGBRegressor(**XGBOOST_HIST_PARAMS),
+                   test_fraction=0.2),
+        classification(xgboost.XGBClassifier(**XGBOOST_HIST_PARAMS),
+                       test_fraction=0.2),
+        classification_binary(xgboost.XGBClassifier(**XGBOOST_HIST_PARAMS),
+                              test_fraction=0.2),
+
         # XGBoost (LINEAR)
         regression(xgboost.XGBRegressor(**XGBOOST_PARAMS_LINEAR)),
         classification(xgboost.XGBClassifier(**XGBOOST_PARAMS_LINEAR)),
diff --git a/tests/test_ast.py b/tests/test_ast.py
index 36b9421a..32c4b4f4 100644
--- a/tests/test_ast.py
+++ b/tests/test_ast.py
@@ -1,3 +1,4 @@
+import numpy as np
 from m2cgen import ast
 
 
@@ -69,3 +70,9 @@ def test_count_all_exprs_types():
         ast.BinNumOpType.MUL)
 
     assert ast.count_exprs(expr) == 27
+
+
+def test_num_val():
+    assert type(ast.NumVal(1).value) == int
+    assert type(ast.NumVal(1, dtype=np.float32).value) == np.float32
+    assert type(ast.NumVal(1, dtype=np.float64).value) == np.float64