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

Decision forest random splitter #2270

Merged
merged 52 commits into from
Mar 23, 2023
Merged

Decision forest random splitter #2270

merged 52 commits into from
Mar 23, 2023

Conversation

inteldimitrius
Copy link
Contributor

Description

Added random splitting strategy.

Changes proposed in this pull request:

  • Added random splitting strategy applied in computation in GPU.
  • Updated splitter class names..
  • Update descriptor and train_context classes.

@inteldimitrius
Copy link
Contributor Author

/intelci: run

@icfaust
Copy link
Contributor

icfaust commented Feb 2, 2023

Any chance you could run the CI Random Forest tests/examples with bootstrap=False and show the results?

@inteldimitrius
Copy link
Contributor Author

Any chance you could run the CI Random Forest tests/examples with bootstrap=False and show the results?

Hi, I added testcases for bootstrap_mode / splitter_mode switching.

Vika-F
Vika-F requested changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@Vika-F Vika-F left a comment

Choose a reason for hiding this comment

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

The main concern is that extra trees still process histogram bins in parallel to compute the split. This needs to be changed. And there are also several minor comments.

docs/source/api/algorithms/ensembles/decision-forest.rst Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/test/batch.cpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/common.hpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_splitter_sp_opt_impl_dpc.cpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_splitter_sp_opt_impl_dpc.cpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_splitter_impl_dpc.cpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_splitter_impl_dpc.cpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_misc_structs.hpp Outdated Show resolved Hide resolved
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Feb 8, 2023
@inteldimitrius
Updates according change requests. In scope of uxlfoundation#2270 pul…
5d9117f 
…l request.
Vika-F
Vika-F requested changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@Vika-F Vika-F left a comment

Choose a reason for hiding this comment

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

Thank you for addressing part of the issues.
But the main change request remains the same as previously: need to remove threading by histogram bins (cgh.parallel_for) for extra trees.

cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_misc_structs.hpp Outdated Show resolved Hide resolved
docs/source/api/algorithms/ensembles/decision-forest.rst Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/common.hpp Show resolved Hide resolved
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Feb 9, 2023
@inteldimitrius
Refactoring for PR uxlfoundation#2270.
bc9e87f
@inteldimitrius
Copy link
Contributor Author

Thank you for addressing part of the issues. But the main change request remains the same as previously: need to remove threading by histogram bins (cgh.parallel_for) for extra trees.

Yes, it is in progress.

inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Feb 9, 2023
@inteldimitrius
Update docs regarding to PR uxlfoundation#2270
a26c678
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Feb 15, 2023
@inteldimitrius
Working version, but prediction score is bad. Please do not consider …
5e12667 
…in PR uxlfoundation#2270
Vika-F
Vika-F requested changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@Vika-F Vika-F left a comment

Choose a reason for hiding this comment

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

The overall logic of the random splitter is correct now, that is great.
But I have several comments regarding the implementation details that need to be addressed or proved that there is no need in fixing them.

cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_kernel_hist_impl_dpc.cpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_kernel_hist_impl.hpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_splitter_sp_opt_impl_dpc.cpp Outdated Show resolved Hide resolved
examples/oneapi/dpc/source/decision_forest/df_cls_hist_batch_random.cpp Outdated Show resolved Hide resolved
examples/oneapi/dpc/source/decision_forest/df_reg_hist_batch.cpp Outdated Show resolved Hide resolved
examples/oneapi/dpc/source/decision_forest/df_reg_hist_batch_random.cpp Outdated Show resolved Hide resolved
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Mar 3, 2023
@inteldimitrius
Updates according change requests. In scope of uxlfoundation#2270 pul…
a396817 
…l request.
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Mar 3, 2023
@inteldimitrius
Refactoring for PR uxlfoundation#2270.
b09819f
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Mar 3, 2023
@inteldimitrius
Update docs regarding to PR uxlfoundation#2270
28c51a3
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Mar 3, 2023
@inteldimitrius
Working version, but prediction score is bad. Please do not consider …
b17b9e9 
…in PR uxlfoundation#2270
@inteldimitrius inteldimitrius force-pushed the decision_forest_random_splitter branch from 7e2ebec to 1de2733 Compare March 3, 2023 18:05
@inteldimitrius
Copy link
Contributor Author

I have rebased onto master this branch and resolved all change requests. The implementation is ready for merge.

Vika-F
Vika-F requested changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@Vika-F Vika-F left a comment

Choose a reason for hiding this comment

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

It seems the adding of using v1::splitter_mode was done a bit incorrectly.
Please see the comments.

cpp/oneapi/dal/algo/decision_forest/common.hpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/common.cpp Outdated Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_splitter_sp_opt_impl.hpp Show resolved Hide resolved
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_splitter_sp_opt_impl.hpp Show resolved Hide resolved
Vika-F
Vika-F approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@Vika-F Vika-F left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

@inteldimitrius inteldimitrius force-pushed the decision_forest_random_splitter branch from c7ad5ee to 3286808 Compare March 22, 2023 15:59
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Mar 22, 2023
@inteldimitrius
Updates according change requests. In scope of uxlfoundation#2270 pul…
03aa3cc 
…l request.
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Mar 22, 2023
@inteldimitrius
Refactoring for PR uxlfoundation#2270.
6a0a7df
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Mar 22, 2023
@inteldimitrius
Update docs regarding to PR uxlfoundation#2270
4b3b896
inteldimitrius added a commit to inteldimitrius/oneDAL that referenced this pull request Mar 22, 2023
@inteldimitrius
Working version, but prediction score is bad. Please do not consider …
5241217 
…in PR uxlfoundation#2270
@samir-nasibli samir-nasibli mentioned this pull request Mar 22, 2023
Merged
@Alexandr-Solovev Alexandr-Solovev merged commit 5e080cf into uxlfoundation:master Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aepanchi aepanchi aepanchi left review comments

@Alexsandruss Alexsandruss Alexsandruss left review comments

@ahuber21 ahuber21 ahuber21 left review comments

@icfaust icfaust icfaust left review comments

@KulikovNikita KulikovNikita KulikovNikita approved these changes

@Vika-F Vika-F Vika-F approved these changes

@samir-nasibli samir-nasibli samir-nasibli approved these changes

@maria-Petrova maria-Petrova Awaiting requested review from maria-Petrova

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

Successfully merging this pull request may close these issues.
9 participants
@inteldimitrius @icfaust @ahuber21 @samir-nasibli @Vika-F @Alexsandruss @KulikovNikita @aepanchi @Alexandr-Solovev