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.

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

Update {regr,classif}.ranger learner for 0.17.0 #321

Merged
merged 7 commits into from
Nov 8, 2024

Conversation

jemus42
Copy link
Member

@jemus42 jemus42 commented Nov 8, 2024

Diff from 0.16.0 to 0.17.0: https://github.com/imbs-hl/ranger/compare/4795a99d43667b47991883fd6ff44e15b2891237..master

Changes to account for:

  • Add poisson splitrule for regr.ranger
  • Add poisson.tau parameter (depends on poisson splitrule being selected)
  • Add beta splitrule which was added earlier but wasn't available in regr.ranger
  • Add "missings" property for {regr,classif}.ranger
  • Add na.action parameter for {regr,classif}.ranger, defaulting to "na.learn"
  • For classif.ranger, let min.node.size and min.bucket also be vectors for class-wise values.
    Would that mean these params need to be p_utys?

Also, ranger changed the default for num.threads, but since this is overriden anyway I didn't touch that part.

I guess the ranger dependency in DESCRIPTION should be bumped to 0.17.0 as well, right?

@be-marc
Copy link
Member

be-marc commented Nov 8, 2024

Also, ranger changed the default for num.threads, but since this is overriden anyway I didn't touch that part.

Yes we should keep this. 2 Cores and future is still a bad choice.

Thanks for updating.

@jemus42
Copy link
Member Author

jemus42 commented Nov 8, 2024

The dev-check is failing because of the boston_housing task, but maybe adjusting relevant tests to use a different task is out of scope for this PR, as it also affects xgboost and lm.

Also from what I can see the only other regression task included in mlr3 by default is mtcars which maybe isn't all too useful for tests? 🤷🏻‍♂️

@be-marc
Copy link
Member

be-marc commented Nov 8, 2024

The dev-check is failing because of the boston_housing task, but maybe adjusting relevant tests to use a different task is out of scope for this PR, as it also affects xgboost and lm.

Yes I will change that.

Also from what I can see the only other regression task included in mlr3 by default is mtcars which maybe isn't all too useful for tests? 🤷🏻‍♂️

Just use mtcars for now. california_housing will be included in the next mlr3 release.

@jemus42
Copy link
Member Author

jemus42 commented Nov 8, 2024

The dev-check is failing because of the boston_housing task, but maybe adjusting relevant tests to use a different task is out of scope for this PR, as it also affects xgboost and lm.

Yes I will change that.

Also from what I can see the only other regression task included in mlr3 by default is mtcars which maybe isn't all too useful for tests? 🤷🏻‍♂️

Just use mtcars for now. california_housing will be included in the next mlr3 release.

So wait, should I change boston_housing -> mtcars in this PR or should I leave it as you'll change that next? 😅

The only other thing left is min.node.size and min.bucket for classif.ranger for which I'm not sure what the best thing to do here is. In most cases it should be a p_int but now it could also be an integer vector. I don't know of any similar cases so I don't know what the preferred solution here would be.

Apart from that I think this is finished so far.

@be-marc
Copy link
Member

be-marc commented Nov 8, 2024

So wait, should I change boston_housing -> mtcars in this PR or should I leave it as you'll change that next? 😅

Merge main and it should be changed

The only other thing left is min.node.size and min.bucket for classif.ranger for which I'm not sure what the best thing to do here is. In most cases it should be a p_int but now it could also be an integer vector. I don't know of any similar cases so I don't know what the preferred solution here would be.

Yes, that makes tuning more complicated. But make it a p_uty().

@jemus42
Copy link
Member Author

jemus42 commented Nov 8, 2024

Gotcha, done.
I more or less guessed on the custom_check though, hope that works.

Currently the autotest fails, but that's due to classif.nnet, not sure what's going on there :/

@be-marc be-marc merged commit d9954ff into mlr-org:main Nov 8, 2024
5 of 6 checks passed
@jemus42 jemus42 deleted the ranger-update branch November 8, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants