-
Notifications
You must be signed in to change notification settings - Fork 7
Pass objective as passible engine argument for rule_fit() #96
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
Conversation
EmilHvitfeldt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice
| # use 4.0 or 4.1 to check with rtools40's older compiler | ||
| - {os: windows-latest, r: 'oldrel-4'} | ||
|
|
||
| - {os: ubuntu-latest, r: 'devel', http-user-agent: 'release'} | ||
| - {os: ubuntu-latest, r: 'release'} | ||
| - {os: ubuntu-latest, r: 'oldrel-1'} | ||
| - {os: ubuntu-latest, r: 'oldrel-2'} | ||
| - {os: ubuntu-latest, r: 'oldrel-3'} | ||
| - {os: ubuntu-latest, r: 'oldrel-4'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these are related to the GHAs failing on older versions, I would use this trick instead: https://github.com/tidymodels/parsnip/pull/1317/files#diff-9c940e8ad2b7bc4c26ec3da57b94bc00e73e2166cfed689da51a4c59bcc0a310L59-L61
| @@ -1,3 +1,20 @@ | |||
| penalties <- 10^(-5:-1) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears not to be used anywhere
| obs_pred <- rf_m_prob |> dplyr::filter(penalty == i) | ||
| for (i in 1:ncol(rf_prob)) { | ||
| expect_equal(obs_pred[[i]], unname(exp_pred[, i])) | ||
| expect_equal(obs_pred[[i]], unname(exp_pred[, i]), tolerance = 0.4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is quite the tolerance!
Related to #95
There are still issues that make it continue to be difficult to make the xrf and rules fits the same.
One other issue is that tidymodels uses a one-hot encoding when creating dummy variables for xgboost while xrf uses the standard full-rank encoding produced by
model.matrix().This PR solves the objective issue and updates some tests for the new xgboost API. The probability-based tests now use a tolerance since their results are very similar. I've removed or commented out tests based on hard class predictions, as we have no way of ensuring that those will be equal (but plan to resolve this discrepancy).