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.

Sign up for GitHub

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

Jump to bottom

Performance improvement for preview ensemble algorithms (RF and ET) #1333

Merged
merged 67 commits into from
Aug 28, 2023
Merged

Performance improvement for preview ensemble algorithms (RF and ET) #1333

merged 67 commits into from
Aug 28, 2023

Conversation

icfaust
Copy link
Contributor

@icfaust icfaust commented Jun 15, 2023 •
edited
Loading

Description

This corrects performance problems associated with estimators_ construction in the random forest and extra trees fit methods for regression and classification, and corrects a performance problem associated with a default construction of a sample_weights vector (which is unnecessary). These yield significant performance improvements.

Changes proposed in this pull request:

  • Add the ability to the 'fit' method to operate without a sample_weights vector, which has impacts on branch of code operation on CPU (15-20% performance impact) and in part prevented GPU operation (sample_weights not supported in oneDAL GPU)
  • Set preview Random Forest Regressor back to using the 'hist' method rather than 'dense' (*already done in other PR)
  • Fix pybind11/python accesses in DFS, streamline slowdowns from abstraction in call() functions
  • Set estimators_ attribute to being lazy evaluated
  • Fixes memory issue when initializing skl_tree_node arrays causing issues in leaf nodes, and creates speedup through C++ initialization rather than copy

@icfaust
Copy link
Contributor Author

icfaust commented Jun 15, 2023

/intelci: run

@icfaust icfaust marked this pull request as ready for review June 15, 2023 15:09
KulikovNikita
KulikovNikita reviewed Jun 19, 2023
View reviewed changes
onedal/primitives/tree_visitor.cpp Outdated Show resolved Hide resolved
onedal/primitives/tree_visitor.cpp Outdated Show resolved Hide resolved
onedal/primitives/tree_visitor.cpp Outdated Show resolved Hide resolved
icfaust and others added 3 commits June 20, 2023 11:06
@icfaust
Update onedal/primitives/tree_visitor.cpp
0faf7eb 
Co-authored-by: KulikovNikita <nikita.kulikov@intel.com>
@icfaust
Update onedal/primitives/tree_visitor.cpp
4c31e8b 
Co-authored-by: KulikovNikita <nikita.kulikov@intel.com>
@icfaust
Update onedal/primitives/tree_visitor.cpp
b343738 
Co-authored-by: KulikovNikita <nikita.kulikov@intel.com>
@icfaust
Copy link
Contributor Author

icfaust commented Jun 20, 2023

/intelci: run

@icfaust
Copy link
Contributor Author

icfaust commented Jun 27, 2023

/intelci: run

ahuber21
ahuber21 previously requested changes Jun 29, 2023
View reviewed changes
onedal/ensemble/forest.cpp Show resolved Hide resolved
onedal/ensemble/forest.py Show resolved Hide resolved
onedal/ensemble/forest.py Show resolved Hide resolved
onedal/ensemble/forest.py Outdated Show resolved Hide resolved
onedal/ensemble/forest.py Outdated Show resolved Hide resolved
onedal/primitives/tree_visitor.cpp Outdated Show resolved Hide resolved
onedal/primitives/tree_visitor.cpp Outdated Show resolved Hide resolved
@Alexsandruss
Copy link
Contributor

@Mergifyio rebase

Alexsandruss
Alexsandruss requested changes Aug 2, 2023
View reviewed changes
onedal/ensemble/forest.py Outdated Show resolved Hide resolved
onedal/primitives/tree_visitor.cpp Outdated Show resolved Hide resolved
onedal/primitives/tree_visitor.cpp Outdated Show resolved Hide resolved
icfaust and others added 3 commits August 2, 2023 13:16
@icfaust @Alexsandruss
Update onedal/primitives/tree_visitor.cpp
7cd70c2 
Co-authored-by: Alexander Andreev <alexander.andreev@intel.com>
@icfaust @Alexsandruss
Update onedal/primitives/tree_visitor.cpp
d411d36 
Co-authored-by: Alexander Andreev <alexander.andreev@intel.com>
@icfaust @Alexsandruss
Update onedal/ensemble/forest.py
9d26f0a 
Co-authored-by: Alexander Andreev <alexander.andreev@intel.com>
@icfaust
Copy link
Contributor Author

icfaust commented Aug 2, 2023

/intelci: run

@icfaust
Copy link
Contributor Author

icfaust commented Aug 2, 2023

/intelci: run

@samir-nasibli
Copy link
Contributor

@icfaust please rebase and enable this tests #1393 , if it's required.

@Alexsandruss
Copy link
Contributor

/intelci: run

@Alexsandruss
Copy link
Contributor

Common PreCommit job with oneapi-src/oneDAL#2441: http://intel-ci.intel.com/ee459771-cca8-f11a-93c5-a4bf010d0e2e

@Alexsandruss Alexsandruss mentioned this pull request Aug 28, 2023
Merged
@Alexsandruss
Copy link
Contributor

http://intel-ci.intel.com/ee45d6a3-be9d-f123-adec-a4bf010d0e2e

@Alexsandruss Alexsandruss dismissed ahuber21’s stale review August 28, 2023 22:13

Comments were addressed

@Alexsandruss Alexsandruss merged commit f8c3b70 into intel:master Aug 28, 2023
16 of 17 checks passed
@icfaust icfaust deleted the previewspeedup branch January 5, 2024 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alexsandruss Alexsandruss Alexsandruss approved these changes

@samir-nasibli samir-nasibli Awaiting requested review from samir-nasibli

@ahuber21 ahuber21 Awaiting requested review from ahuber21

@KulikovNikita KulikovNikita Awaiting requested review from KulikovNikita

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@icfaust @Alexsandruss @samir-nasibli @ahuber21 @KulikovNikita