[bug] fix MT2203 RNG non-uniformity and random bin indices in decision forest training #2592
+17
−18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
icfaust
requested review from
razdoburdin,
ahuber21 and
avolkov-intel
as code owners
|
/intelci: run |
|
special private CI run with mentioned tests enabled: http://intel-ci.intel.com/ee886edb-4adb-f114-a7aa-a4bf010d0e2e |
|
Performance comparisons are available upon request |
|
The test_distribution for ExtraTreesRegressor is not fixed. This requires further analysis. |
icfaust
changed the title
icfaust
changed the title
|
special private CI run with mentioned tests enabled: http://intel-ci.intel.com/ee891bc7-4b31-f12d-a430-a4bf010d0e2e |
|
/intelci: run |
|
Private CI shows this causes an issue with ExtraTreesClassification now, will need to investigate |
KulikovNikita
approved these changes
Dec 13, 2023
Comment on lines
+448
to
+449
| RNGsInst<unsigned int, cpu> rng; | ||
| services::internal::TArray<unsigned int, cpu> temp(burn); |
There was a problem hiding this comment.
Suggested change
| RNGsInst<unsigned int, cpu> rng; | |
| services::internal::TArray<unsigned int, cpu> temp(burn); | |
| RNGsInst<uint32_t, cpu> rng; | |
| services::internal::TArray<uint32_t, cpu> temp(burn); |
|
/intelci: run ml_benchmarks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Each MT2203 RNG engine is independently uniform when taking samples. However, when two or more engines are compared, the initial aggregated random numbers are not uniform. Because the randomness between trees needs to be guaranteed for the decision forest algorithm (each with its own RNG engine), a imperceptible performance loss is introduced to burn RNG values to where the engine collection is empirically uniform.
The second issue is a problem with the binary search associated with finding a split for ExtraTrees (regressor and classifier). The search failed to find the largest bin left edge in its current orientation, and so it has been switched to always guarantee a valid split. This change comes from the ambiguity of using a binning approach with the Extra Trees algorithm definition. All use of the .min parameter are removed, and so it is completely removed from IndexedFeatures and initial binning scripts.
This will fix the following deselected_tests from sklearnex:
tests/test_multioutput.py::test_classifier_chain_tuple_order
ensemble/tests/test_forest.py::test_distribution
However, this is changing the determinism of the trees used in the sklearnex tests, which means some tests which passed by chance could now fail.
This non-uniformity negatively impacts both the random forest in the bootstrapping process, and in extra trees in the initial chosen splits.
Changes proposed in this pull request: