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

Add test for xgboost modelbuiler #1359

Merged
merged 9 commits into from
Jul 27, 2023

Conversation

avolkov-intel
Copy link
Contributor

Description

In this PR #1350 fix for xgboost modelbuilder was presented. The issue was about inference of model fitted with early_stopping_rounds parameter. In this PR I present the test that shows that there is no disrepancy between xgboost and xgboost modelbuilders.

@avolkov-intel
Copy link
Contributor Author

/intelci: run

@avolkov-intel
Copy link
Contributor Author

@razdoburdin, please, review this PR

@@ -0,0 +1,119 @@
#===============================================================================
# Copyright 2014 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Copyright 2014 Intel Corporation
# Copyright 2023 Intel Corporation

@napetrov
Copy link
Contributor

@avolkov-intel - let's not use examples as mean for testing - this is foe sure doesn't worth separate example to be created

@samir-nasibli samir-nasibli changed the title Add test for xgboost modelbuiler Add example for xgboost modelbuiler Jul 12, 2023
@samir-nasibli
Copy link
Contributor

@avolkov-intel please add unit test for that feature. Let's do it on separate PR

@samir-nasibli
Copy link
Contributor

@avolkov-intel please also update doc/daal4py/model-builders.rst for this example.

Copy link
Contributor

@samir-nasibli samir-nasibli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address CI fails


# Training
xgb_clf = xgb.XGBClassifier(**params)
xgb_clf.fit(X_train, y_train, eval_set=[(X_test, y_test)])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want the validation set is printed to stdout at each boosting stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should provide eval_set to fit xgboost with early_stopping_rounds (this feature is used to avoid overfitting so dataset different from train should be provided)

@avolkov-intel
Copy link
Contributor Author

@avolkov-intel please add unit test for that feature. Let's do it on separate PR

I suppose I will just remove example and add test in this PR since we do not need to add new example as Nikolay said

@avolkov-intel
Copy link
Contributor Author

/intelci: run

@avolkov-intel avolkov-intel changed the title Add example for xgboost modelbuiler Add test for xgboost modelbuiler Jul 18, 2023
@avolkov-intel
Copy link
Contributor Author

/intelci: run

@avolkov-intel
Copy link
Contributor Author

@razdoburdin, please review the PR

Copy link
Contributor

@razdoburdin razdoburdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test itself is fine. The problem is the failure of the private-ci. XGBoost is not installed in private-ci environment, so we need to skip the test in this case somehow.

@napetrov
Copy link
Contributor

@razdoburdin @homksei - might be it worth to just add xgboost package internally?

@razdoburdin
Copy link
Contributor

@razdoburdin @homksei - might be it worth to just add xgboost package internally?

it will be the best option for us

@avolkov-intel
Copy link
Contributor Author

/intelci: run

hasattr(d4p, 'gbt_classification_prediction'),
daal_check_version(((2021, 'P', 1)))]), reason)
@unittest.skipUnless(importlib.util.find_spec('xgboost')
is not None, 'xgoost library is not installed')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
is not None, 'xgoost library is not installed')
is not None, 'xgboost library is not installed')

@razdoburdin razdoburdin added the testing Tests for sklearnex/daal4py/onedal4py & patching sklearn label Jul 21, 2023
@avolkov-intel
Copy link
Contributor Author

/intelci: run

Copy link
Contributor

@samir-nasibli samir-nasibli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that this tests are passed:

test_earlystop (test_xgboost_mb.XgboostModelBuilder) ... skipped 'xgboost library is not installed'

Please address this issue first

@samir-nasibli
Copy link
Contributor

Sorry, I see that this test passed on public CI:

test_earlystop (test_xgboost_mb.XgboostModelBuilder) ... DAAL version: (2023, 'P', 101)

Copy link
Contributor

@samir-nasibli samir-nasibli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@avolkov-intel avolkov-intel merged commit f6ab585 into intel:master Jul 27, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
model builders testing Tests for sklearnex/daal4py/onedal4py & patching sklearn
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants