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
add caret tidiers #344
add caret tidiers #344
Conversation
Failure message was I don't think my PR has anything to do with those packages, I added |
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.
Looks useful, can't wait to give it a go myself! Requested some small changes.
Not sure why the build is failing, will have to look into that next week.
R/caret_tidiers.R
Outdated
#' \code{glance} method. | ||
#' | ||
#' @param x An object of class \code{confusionMatrix} | ||
#' @param show_class A boolean of whether to show the values for class specific |
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.
I'd use "logical" instead of boolean since that's more standard for R.
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.
I'd also rename show_class
to by_class
.
R/caret_tidiers.R
Outdated
#' to FALSE, result will only show accuracy and kappa. | ||
#' @param ... extra arguments (not used) | ||
#' | ||
#' @return A data.frame with one or more of the following columns: |
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.
Should return a tibble!
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.
I think I need to fix the durbinWatsonTest as well then.
This looks good! I'd like to merge but I need to think a little bit about the implications of adding |
It's giving me errors on my test script saying that str_to_lower couldn't be found, but it's from the stringr package. |
WRT to builds failing on travis-ci, this message is at the end of all the failures I"ve just checked: |
At the moment it's okay for the Travis build to fail, that's on us, but AppVeyor does need to pass. Pretty busy the next couple of days but will get back to this at some point this week. |
Thanks @atyre2 I saw that as well. Thanks for the response @alexpghayes, I'll do whatever is needed to fix this, but I'm really not sure what's going on. |
Awesome! Thanks for your persistence with this! |
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. |
implements #270