Skip to content

Correct translation of RandomForest criterion hyperparameter #6363

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
Merged
merged 12 commits into from
Feb 26, 2025
Merged

Correct translation of RandomForest criterion hyperparameter #6363

merged 12 commits into from
Feb 26, 2025

Conversation

wphicks
Copy link
Contributor

@wphicks wphicks commented Feb 24, 2025

No description provided.

@wphicks wphicks requested a review from a team as a code owner February 24, 2025 23:17
@wphicks wphicks requested review from cjnolet and betatim February 24, 2025 23:17
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Feb 24, 2025
@wphicks wphicks added bug Something isn't working non-breaking Non-breaking change labels Feb 24, 2025
@dantegd dantegd mentioned this pull request Feb 25, 2025
Merged
24 tasks
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 25, 2025
@dantegd
Backport PR rapidsai#6363
c61d732
betatim
betatim reviewed Feb 25, 2025
View reviewed changes
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 25, 2025
@dantegd
Backport PR rapidsai#6363
5016dca
betatim
betatim reviewed Feb 25, 2025
View reviewed changes
python/cuml/cuml/ensemble/randomforestregressor.pyx Outdated
"criterion": "NotImplemented",
"criterion": {
"friedman_mse": "NotImplemented",
"absolute_error": "NotImplemented",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"absolute_error": "NotImplemented",
"absolute_error": "NotImplemented",
"squared_error": "mse",

So that RandomForestRegressor() and RandomForestRegressor(criterion="squared_error") both work. Explicitly passing the constructor arg's default value might happen during grid searching

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check out the latest implementation. I think this may have been based on 4c8a80b, but the original intent was that this translation should not be necessary. I believe that is true in the current implementation.

betatim
betatim reviewed Feb 25, 2025
View reviewed changes
@betatim
Copy link
Member

betatim commented Feb 25, 2025

Just to double check: if the user passes criterion=blah, does that get used in favour of the value of split_criterion? I had a quick look (aka grep for split_criterion) at the code but couldn't find where the translation/precedent happens :-/

@rapidsai rapidsai deleted a comment from dantegd Feb 25, 2025
@wphicks
Copy link
Contributor Author

wphicks commented Feb 25, 2025

Deleted the merge comment because I think 4c8a80b broke this functionality. Want to make sure we take a closer look before proceeding.

@wphicks wphicks marked this pull request as draft February 25, 2025 14:37
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Feb 25, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@wphicks wphicks requested review from dantegd, betatim and hcho3 February 25, 2025 18:10
@wphicks wphicks marked this pull request as ready for review February 25, 2025 18:44
jcrist
jcrist approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@jcrist jcrist left a comment

Choose a reason for hiding this comment

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

Approving in case we just want to merge it (logic looks correct). Noted two small nits in the implementation, leaving it up to you if you want to address them or not.

csadorf
csadorf approved these changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Logic looks fine to me. I did not check whether the actual mapping of supported criteria is correct.

dantegd added a commit to dantegd/cuml that referenced this pull request Feb 25, 2025
@dantegd
Backport PR rapidsai#6363 redux
3468eef
@dantegd
Copy link
Member

dantegd commented Feb 26, 2025

/merge

@rapids-bot rapids-bot bot merged commit 0dc3bce into rapidsai:branch-25.04 Feb 26, 2025
64 of 65 checks passed
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 26, 2025
@dantegd
Backport PR rapidsai#6363
6dac1c4
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 26, 2025
@dantegd
Backport PR rapidsai#6363
51fef8d
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 26, 2025
@dantegd
Backport PR rapidsai#6363 redux
249f49c
raydouglass pushed a commit that referenced this pull request Feb 28, 2025
@dantegd @wphicks
Backport release 25.04 PRs for patch release version 25.02.01 (#6329)
9677544 
PRs being backported: 

- [x] #6234
- [x] #6306
- [x] #6320
- [x] #6319
- [x] #6327
- [x] #6333
- [x] #6142 
- [x] #6223
- [x] #6235
- [x] #6317 
- [x] #6331
- [x] #6326
- [x] #6332
- [x] #6347
- [x] #6348
- [x] #6337
- [x] #6355
- [x] #6354
- [x] #6322
- [x] #6353
- [x] #6359
- [x] #6364
- [x] #6363
- [x] [FIL BATCH_TREE_REORG fix for SM90, 100 and
120](a3e419a)

---------

Co-authored-by: William Hicks <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csadorf csadorf csadorf approved these changes

@jcrist jcrist jcrist approved these changes

@cjnolet cjnolet Awaiting requested review from cjnolet cjnolet is a code owner automatically assigned from rapidsai/cuml-python-codeowners

@dantegd dantegd Awaiting requested review from dantegd

@betatim betatim Awaiting requested review from betatim

@hcho3 hcho3 Awaiting requested review from hcho3

Assignees
No one assigned
Labels
bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wphicks @betatim @dantegd @csadorf @hcho3 @jcrist