restore forest deselection from normalize sample weights#3292
restore forest deselection from normalize sample weights#3292ethanglaser wants to merge 6 commits into
Conversation
ethanglaser
requested review from
ahuber21,
avolkov-intel,
david-cortes-intel,
icfaust,
razdoburdin and
yuejiaointel
as code owners
There was a problem hiding this comment.
Pull request overview
This PR restores scikit-learn test coverage for oneDAL-backed forests by fixing sample-weight handling in onedal decision forests so that feature-importance tests for ExtraTrees are no longer deselected.
Changes:
- Normalize
sample_weightbefore forest training to improve invariance under weight scaling. - Re-enable previously deselected
ExtraTreesRegressorfeature-importance tests (float32/float64) by removing them fromdeselected_tests.yaml.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
onedal/ensemble/forest.py |
Adds pre-fit sample-weight normalization intended to prevent weight-scale sensitivity in oneDAL forest training. |
deselected_tests.yaml |
Removes deselections for ExtraTreesRegressor feature-importance tests so they run again in CI. |
| _, xp, _ = _get_sycl_namespace(sample_weight) | ||
| # float() keeps the divisor a Python scalar so the division is valid | ||
| # across array namespaces (e.g. array_api_strict rejects a foreign | ||
| # numpy scalar) and preserves sample_weight's dtype. | ||
| max_weight = float(xp.max(sample_weight)) | ||
| if max_weight > 0: | ||
| sample_weight = sample_weight / max_weight |
|
@ethanglaser @Alexandr-Solovev This should be done on the oneDAL side, together with other obvious tricks like standardizing 'y', for both CPU and GPU. |
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:
|
| - model_selection/tests/test_classification_threshold.py::test_tuned_threshold_classifier_cv_zeros_sample_weights_equivalence >=1.5 | ||
|
|
||
| # Deselections for 2025.0 | ||
| - ensemble/tests/test_forest.py::test_importances[ExtraTreesRegressor-squared_error-float64] |
There was a problem hiding this comment.
@ethanglaser This test shouldn't even be getting patched in sklearn1.9 onwards:
https://github.com/scikit-learn/scikit-learn/blob/fa7eddec61a9f336a27e1519161c23b357af138c/sklearn/ensemble/tests/test_forest.py#L339
See the conditions:
In which job is this failure observed?
There was a problem hiding this comment.
Not sure, this would have been deselected over a year ago
There was a problem hiding this comment.
But these are the same test aside from fp type
|
oneDAL update is in progress: https://github.com/uxlfoundation/oneDAL/compare/main...ethanglaser:dev/eglaser-forest-fix?expand=1 |
ethanglaser
changed the title
|
Would merge this after the changes from uxlfoundation/oneDAL#3674 are incorporated into the oneDAL build that github actions fetches |
Description
Fixes to forest sample weight handling to undo recent forest deselection (#3231). Also addresses older extratrees deselected test.
Checklist:
Completeness and readability
Testing
Performance