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

use ldply instead of lapply to tidy aovlist's #377

Merged
merged 8 commits into from Jun 21, 2018

Conversation

mvevans89
Copy link
Contributor

this changes the way tidy is used to tidy aovlist's (such as within-subject error structures) to close issue #299

Copy link
Collaborator

@alexpghayes alexpghayes left a comment

Choose a reason for hiding this comment

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

Thanks so much for this PR! I made a couple comments on the code, but this looks great overall.

function(a) dplyr::mutate(ret[[a]], stratum = a)
)
ret <- do.call("rbind", ret)
ret <- plyr::ldply(x, tidy, .id = "stratum")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're looking to move away from plyr at the moment. Would you be willing to use purrr here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me. I'll switch it up.

#'
#' @importFrom plyr ldply
#'
#' @import dplyr
Copy link
Collaborator

Choose a reason for hiding this comment

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

dplyr and plyr get imported package-wide in DESCRIPTION, so I believe these aren't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I'll remove them.

@@ -0,0 +1,15 @@
context("anova tidiers")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Regression tests! Awesome!


test_that("tidy.aovlist works"){
aovlistfit <- aov(mpg ~ wt + disp + Error(drat), mtcars)
td <- suppressWarnings(tidy(aovlistfit))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What kind of warnings are getting suppressed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked and there actually are none. Warning suppression was just a hold over from copying the format of the test-lm.R. I'll take it out

@alexpghayes
Copy link
Collaborator

Don't worry about the build failure at the moment, BTW, that's my fault and I need to fix it.

@alexpghayes
Copy link
Collaborator

Changes look good. Will merge as soon as I sort out the lavaan stuff causing the build failure.

@mvevans89 mvevans89 requested a review from dgrtwo as a code owner Jun 18, 2018
@alexpghayes
Copy link
Collaborator

Okay, think we're good to merge. Feel free to add yourself as a contributor to DESCRIPTION if you'd like!

@alexpghayes alexpghayes merged commit 24fc3c5 into tidymodels:master Jun 21, 2018
@github-actions
Copy link

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 Mar 11, 2021
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.

None yet

2 participants