Model builders API update

[razdoburdin]

I want to start discussion about changing the Model builders API.
The main challenge is support of both scikit-learn style estimators and non-scikit style booster objects.
In the presented PR I added a new class GBTDAALModel for python interface in non-scikit style. It allow to hide all worldly syntax from the old API.

In case of using XGBoost, for regression tasks user-side code changes from:
d4p_model = daal4py.get_gbt_model_from_xgboost(booster)
d4p_prediction = daal4py.gbt_regression_prediction().compute(X_test, d4p_model).prediction
to
d4p_model = daal4py.mb.convert_model(booster)
d4p_prediction =d4p_model.predict()

In case of classification problem user-side code changes from:
d4p_model = daal4py.get_gbt_model_from_xgboost(booster)
d4p_prediction=daal4py.gbt_classification_prediction(nClasses=n_classes).compute(X_test, d4p_model).prediction
to
d4p_model = daal4py.mb.convert_model(booster)
d4p_prediction = d4p_model.predict()

For support of scikit-style estimators, I updated GBTDAALClassifier and GBTDAALRegressor classes.
One can use them like this (example for XGBoost, regression task):
from daal4py.sklearn.ensemble import GBTDAALRegressor
reg = xgb.XGBRegressor()
reg.fit(X, y)
d4p_predt = GBTDAALRegressor.convert_model(reg).predict(X)

[napetrov]

the only thing that i don't fully like - CamelCase on .GbtModel. Might be GBTModel would be better?

[razdoburdin]

the only thing that i don't fully like - CamelCase on .GbtModel. Might be GBTModel would be better?

fixed

[inteldimitrius]

The rest looks good to me!

[razdoburdin]

Dear all,
I have updated the PR. Currently, the scikit-style estimators are supported. Please see the updated description for details.

[inteldimitrius]

MacOS and Linux CI checks are failing. Is it okay?

[razdoburdin]

Thanks for noting this!
I have reproduced the test failure locally. The problem is in NAN test in predict() methods for estimators. As far as NAN are now supported, I removed the NAN test. But the sklearn.check_estimator requires this check. From another hand XGBRegressor supports NAN also and passes this check.

@Alexsandruss, have you any ideas how to deal with it?

[Alexsandruss]

NaN errors are not the only ones:

======================================================================
ERROR: test_gbt_cls_model_create_from_catboost_batch (test_examples.TestExNpyArray)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vsts/work/1/s/tests/test_examples.py", line 96, in testit
    result = self.call(ex)
  File "/home/vsts/work/1/s/tests/test_examples.py", line 269, in call
    return ex.main(readcsv=np_read_csv)
  File "/home/vsts/work/1/s/examples/daal4py/gbt_cls_model_create_from_catboost_batch.py", line 73, in main
    daal_prediction = daal_predict_algo.compute(X_test, daal_model)
TypeError: Argument 'model' has incorrect type (expected daal4py._daal4py.gbt_classification_model, got tuple)

[Alexsandruss]

Snake case is used for file names in python

[razdoburdin]

done

[Alexsandruss]

Implement nan tag dispatching for base class:

def _more_tags(self):
    return {"allow_nan": self.allow_nan_}

[razdoburdin]

Implement nan tag dispatching for base class:

def _more_tags(self):
    return {"allow_nan": self.allow_nan_}

done

[Alexsandruss]

Method is not used. Same for second example.

Suggested change

	def main(readcsv=pd_read_csv, method='defaultDense'):
	def main(readcsv=pd_read_csv):

[razdoburdin]

Method is not used. Same for second example.

done

[razdoburdin]

/intelci: run

[napetrov]

@razdoburdin test_model_builders_xgboost/ test_model_builders_lightgbm are failing internally

[napetrov]

/intelci: run

[Alexsandruss]

fptype should be deduced from input data.

[napetrov]

/intelci: run

[samir-nasibli]

@razdoburdin you have changed examples names, removed suffix _batch. Examples runner will ignore this such file names https://github.com/intel/scikit-learn-intelex/blob/master/tests/run_examples.py#L219:L222 .

[samir-nasibli]

Please rename your examples files and update runner script

Suggested change

	req_library['model_builders_lightgbm.py'] = ['lightgbm']
	req_library['model_builders_xgboost.py'] = ['xgboost']
	req_library['model_builders_catboost.py'] = ['catboost']
	req_library['model_builders_lightgbm_batch.py'] = ['lightgbm']
	req_library['model_builders_xgboost_batch.py'] = ['xgboost']
	req_library['model_builders_catboost_batch.py'] = ['catboost']

[napetrov]

No. Batch is strange and miningless suffix. If runner will not pick up them, then launcher should be fixed

[samir-nasibli]

Let's decide how we can group-by this kind of examples then.
All examples scikit-learn-intelex/examples/ are all grouped logically into 4 groups: spmd.py, streaming.py, stream.py, batch.py.
If none of them is suitable, then:

• we will come up with a new suffix for such examples.
• update whole examples runner logic

[razdoburdin]

I have just made a brief look in the run_examples.py. May we just add some sort of default behavior in case the filename doesn't end on *spmd.py, *streaming.py, *stream.py or *batch.py?
As far as I see we could switch to this default behavior for all the *_batch.py and *_stream.py files. In my mind, it will make naming of examples more clear.

[napetrov]

@razdoburdin i like that idea more, than using suffixes.

@samir-nasibli - those are logical only if you look on this from some testing perspective. From end user perspective this looks strange.

I would go with base examples without batch because it make no sense to specify this. On stream/streaming - looks like this should have be single group. And for spmd - let's leave it as is

@Alexsandruss, @KulikovNikita - your thoughts?

[samir-nasibli]

I have just made a brief look in the run_examples.py. May we just add some sort of default behavior in case the filename doesn't end on *spmd.py, *streaming.py, *stream.py or *batch.py?

Sure we can. That means that we need update examples runner logic little bit (second option).