Extend modelbuilders test set#2856
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Wouldn't it make more sense to have tests like this on the oneDAL repository? |
| # ============================================================================ | ||
| # Tree depth utility functions for deep trees testing | ||
| # ============================================================================ | ||
| def compute_xgb_tree_depth(booster: xgb.Booster) -> int: |
There was a problem hiding this comment.
Since the idea is to test some internal detail of the implementation that doesn't anyhow depend on the input converter, how about checking only a single GBT library? Perhaps a random forest from sklearn converted with treelite could be used.
There was a problem hiding this comment.
We actually have some existing tests related to xgboost and catboost including SHAP tests, it would require much more effort to create tests using RandomForest and TreeLite compared to updating existing tests
There was a problem hiding this comment.
Then how about leaving serialized model objects instead of re-creating them every time? These tests are already having issues in CIs due to taking too long to execute, and some are currently skipped due to timings.
There was a problem hiding this comment.
I think this would make the testing more complicated, not sure if we use serialized models in other tests. Although, I agree that modelbuilders tests take quite a lot of time, the relative slowdown after introducing tests in this PR is small. That's why let's create a follow up for speeding up this set of tests
There was a problem hiding this comment.
From what I can see in the CI jobs here compared to those of recent commits, it adds roughly 1 additional minute in the CI servers.
avolkov-intel
requested review from
ahuber21,
ethanglaser,
icfaust and
yuejiaointel
as code owners
|
Please address linter fail |
|
/intelci: run |
|
/intelci: run |
avolkov-intel
added
the
testing
3 participants
Description
Since we plan an update to internal modelbuilders layout by introducing hybrid layout (uxlfoundation/oneDAL#3455) we need to extend test set.
In the new layouts we would create full binary tree layout for levels <= 10 but after that we will store nodes in dense format. To check, that this implementation doesn't have any bugs we need to add tests where deep trees are used.
This test introduces new tests where trees with depth >=12 or >=15 created.
Checklist:
Completeness and readability
Testing