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

Implement apply() in FIL #5307

Closed
wants to merge 12 commits into from
Closed

Implement apply() in FIL #5307

wants to merge 12 commits into from

Conversation

hcho3
Copy link
Contributor

@hcho3 hcho3 commented Mar 28, 2023 •
edited
Loading

No description provided.

@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Mar 28, 2023
@hcho3 hcho3 marked this pull request as ready for review March 30, 2023 23:16
@hcho3 hcho3 requested review from a team as code owners March 30, 2023 23:16
@hcho3 hcho3 added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Mar 30, 2023
@hcho3
Copy link
Contributor Author

hcho3 commented Mar 31, 2023

@wphicks I found out that test_filex.py was not being run by the CI. I added it to the list of pytests to run.

wphicks
wphicks reviewed Mar 31, 2023
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

Looking good! One implementation detail that I think could be improved and a question about method naming.

cpp/include/cuml/experimental/fil/decision_forest.hpp Outdated Show resolved Hide resolved
cpp/include/cuml/experimental/fil/detail/decision_forest_builder.hpp Outdated Show resolved Hide resolved
cpp/include/cuml/experimental/fil/detail/evaluate_tree.hpp Outdated Show resolved Hide resolved
cpp/include/cuml/experimental/fil/detail/evaluate_tree.hpp Show resolved Hide resolved
python/cuml/experimental/fil/fil.pyx Outdated Show resolved Hide resolved
python/cuml/experimental/fil/fil.pyx Outdated Show resolved Hide resolved
@hcho3 hcho3 changed the base branch from branch-23.04 to branch-23.06 March 31, 2023 17:47
@hcho3 hcho3 requested review from a team as code owners March 31, 2023 17:47
@hcho3 hcho3 changed the base branch from branch-23.06 to branch-23.04 March 31, 2023 17:47
@hcho3
Copy link
Contributor Author

hcho3 commented Mar 31, 2023 •
edited
Loading

I'll close this PR and open a new PR targeting 23.06.

EDIT. I'll have to wait until #5315 is merged.

@hcho3 hcho3 changed the title Implement predict_leaf() in FIL Implement apply() in FIL Apr 1, 2023
@ajschmidt8 ajschmidt8 removed the request for review from a team April 3, 2023 17:27
@hcho3 hcho3 mentioned this pull request Apr 12, 2023
Merged
@hcho3
Copy link
Contributor Author

hcho3 commented Apr 12, 2023

Closing in favor of #5358

@hcho3 hcho3 closed this Apr 12, 2023
@hcho3 hcho3 deleted the predict_leaf branch April 12, 2023 17:55
rapids-bot bot pushed a commit that referenced this pull request May 26, 2023
@hcho3
Implement apply() in FIL (#5358)
36c1ea9 
Replaces #5307

Depends on #5365

Authors:
  - Philip Hyunsu Cho (https://github.com/hcho3)
  - Dante Gama Dessavre (https://github.com/dantegd)

Approvers:
  - William Hicks (https://github.com/wphicks)

URL: #5358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wphicks wphicks wphicks left review comments

Assignees
No one assigned
Labels
CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hcho3 @wphicks