Skip to content

Correct translation of RandomForest criterion hyperparameter#6363

Merged
rapids-bot[bot] merged 12 commits intorapidsai:branch-25.04from
wphicks:bug-rf_param_translation
Feb 26, 2025
Merged

Correct translation of RandomForest criterion hyperparameter#6363
rapids-bot[bot] merged 12 commits intorapidsai:branch-25.04from
wphicks:bug-rf_param_translation

Conversation

@wphicks
Copy link
Copy Markdown
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 betatim and cjnolet 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 added a commit to dantegd/cuml that referenced this pull request Feb 25, 2025
Comment thread python/cuml/cuml/ensemble/randomforestclassifier.pyx Outdated
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 25, 2025
"criterion": "NotImplemented",
"criterion": {
"friedman_mse": "NotImplemented",
"absolute_error": "NotImplemented",
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread python/cuml/cuml/ensemble/randomforestclassifier.pyx
@betatim
Copy link
Copy Markdown
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
Copy Markdown
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 link
Copy Markdown

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 betatim, dantegd and hcho3 February 25, 2025 18:10
@wphicks wphicks marked this pull request as ready for review February 25, 2025 18:44
Copy link
Copy Markdown
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.

Comment thread python/cuml/cuml/ensemble/randomforest_common.pyx
Comment thread python/cuml/cuml/ensemble/randomforest_common.pyx
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member

dantegd commented Feb 26, 2025

/merge

@rapids-bot rapids-bot Bot merged commit 0dc3bce into rapidsai:branch-25.04 Feb 26, 2025
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 26, 2025
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 26, 2025
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 26, 2025
raydouglass pushed a commit that referenced this pull request Feb 28, 2025
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 <whicks@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants