Skip to content

move mtry_prop from rules to dials #233

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.

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

Merged
merged 4 commits into from
May 25, 2022
Merged

move mtry_prop from rules to dials #233

merged 4 commits into from
May 25, 2022

Conversation

simonpcouch
Copy link
Contributor

So that bonsai need not take on a rules dependency for this parameter!

Bumping version so we can require a specific dev version of this package in rules. Will be accompanied by a PR to rules shortly. :)

Bumping version so we can require a specific dev version of this package in `rules`. :)
@simonpcouch
Copy link
Contributor Author

Update: I don't think bonsai actually needs to make use of the mtry_prop parameter. Leaving this and tidymodels/rules#54 open in case it's otherwise helpful to make this switch, but feel free to close if not!

@hfrick
Copy link
Member

hfrick commented May 11, 2022

have we settled on the API where the default is mtry is a count but you can switch to a proportion with counts = FALSE in set_engine()? (as you did for lightgbm, and already has been done for xgboost)

If so, would it make sense to switch rules over to that API as well? It would be nice to have a consistent approach if it's the same problem. cc @topepo

Copy link
Member

@hfrick hfrick left a comment

Choose a reason for hiding this comment

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

I spoke with Max and the plan is now:

  • update rules to use the counts API tidymodels/rules#55
  • move mtry_prop() from rules to dials so it's still usable/maintained but add a note that our convention is to use counts and encourage people to use that as well when developing extensions.

Could you add such a note, please?

@simonpcouch
Copy link
Contributor Author

Thanks for the review. :) Added a docs section clarifying our convention there.

@simonpcouch simonpcouch requested a review from hfrick May 24, 2022 18:30
@hfrick hfrick merged commit c15aac7 into main May 25, 2022
@hfrick hfrick deleted the mtry-prop branch May 25, 2022 09:26
@github-actions
Copy link

github-actions bot commented Jun 9, 2022

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants