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

metafor::rma tidiers - work in progress #674

Merged
merged 26 commits into from Apr 8, 2019
Merged

metafor::rma tidiers - work in progress #674

merged 26 commits into from Apr 8, 2019

Conversation

softloud
Copy link
Contributor

@softloud softloud commented Apr 4, 2019

These tidiers are still work in progress with @malcolmbarrett.

Will ping you @alexpghayes, when we're ready for you to take a look.

resources

I recommend starting with strict = FALSE for all the tests. Then do tidy() and glance() with strict = TRUE. Only then do augment() with strict = TRUE. augment() is astonishingly painful to get right.

— alex hayes (@alexpghayes) April 5, 2019

tasks

  • convert Malcolm's script to tests
  • figure out why methods aren't being picked up
  • mine tidyverse contributor post for git tips
  • add us as contributors
  • Malcolm to add his ORCID

@softloud softloud changed the title ported @malcolmbarrett's testing script metafor::rma tidiers Apr 6, 2019
@softloud softloud changed the title metafor::rma tidiers metafor::rma tidiers - work in progress Apr 6, 2019
@softloud
Copy link
Contributor Author

softloud commented Apr 6, 2019

Tests are failing because the methods aren't recognised.

My new attained fluffy duck understanding of S3 is failing me. I think there's stuff in the roxygen documentation in the other tidier files that will fix this?

Is there a guide or template for this documentation, @alexpghayes?

Or perhaps you know how to deal with this, @malcolmbarrett?

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.

Hey this is looking great! I left lots of comments but am generally quite happy with this and just want to make sure it's top-notch and polished so lots of people use it!

R/rma-tidiers.R Outdated
#' output?
#' @param ... additional arguments
#' @param measure measure type. See [metafor::rma()]
#' @param ... Additional arguments
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do these arguments get based too? Link to the documentation of that function to make it easier for users trying to track that down.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, they actually aren't used but need to be there to match the generic. I seem to recall some methods in broom saying something like Additional arguments (not used)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If they aren't getting used the standard way to document them is with @template param_unused_dots

NEWS.md Show resolved Hide resolved
R/rma-tidiers.R Outdated
@@ -0,0 +1,222 @@
#' Tidying methods for meta-analyis objects
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use the templating trick to pull in standard boilerplate. i.e.:

#' @templateVar class rma
#' @template title_desc_tidy

rather than this first line.

R/rma-tidiers.R Outdated
#' @param ... Additional arguments
#' @param measure Measure type. See [metafor::rma()]
#'
#' @return A `tibble`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Take a look at https://github.com/tidymodels/broom/blob/master/R/betareg-tidiers.R#L8 for an example of how to document the tidier return. There are three separate functions return_tidy(), ..., return_augment() to look at. Also I definitely need to document how to use these better.

R/rma-tidiers.R Outdated
#'
#' tidy(meta_analysis)
#'
#' @rdname tidiers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#' @rdname tidiers
#' @rdname metafor_tidiers

R/rma-tidiers.R Outdated
purrr::discard(~length(.x) >= 2) %>%
# change to tibble with correct column and row names
as.data.frame() %>%
tibble::as_tibble() %>%
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think tibble::remove_rownames() is necessary if you use tibble::as_tibble()?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right! These are leftover from when I first wrote these, which was prior to broom using tibble

R/rma-tidiers.R Show resolved Hide resolved
R/rma-tidiers.R Outdated
ret <- tibble::as_tibble(ret) %>%
tibble::remove_rownames()

if (all(ret$.rownames == seq_along(ret$.rownames))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's a has_rownames() helper somewhere that you can use here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to change the approach in these lines to make it a little clearer what is happening because the rownames aren't coming from the usual place in this model

@@ -0,0 +1,207 @@
context("rma")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind structuring the tests based on method? This is what the other tests files do. For an example you could look at: https://github.com/tidymodels/broom/blob/master/tests/testthat/test-aer.R#L33


test_that(("returns tibble for MH"), {
check_tidy_output(tidy(res.MH))
# check_augment_outputs(glance(res.MH))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented out tests can be confusing. I would either note why this is commented out or delete the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I didn't notice this myself. check_augment_outputs isn't a function in modeltests yet, right? we can replace with a simpler check for tibble output in the augment functions

Copy link
Contributor

Choose a reason for hiding this comment

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

Or should we use modeltests::check_augment_function?

Copy link
Contributor

Choose a reason for hiding this comment

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

After exploring modeltests::check_augment_function a bit, I see I'd run into the problem with the data argument even when strict = FALSE

Copy link
Collaborator

Choose a reason for hiding this comment

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

The augment checks currently are designed to make sure prediction on newdata doesn't blow up, more or less. I would use check_tidy_output() and check_glance_outputs() and then write some simple custom checks in for the augment.rma() method.

@malcolmbarrett
Copy link
Contributor

malcolmbarrett commented Apr 8, 2019

Changes implemented, @alexpghayes! Tests for the rma tidiers now all pass, as well (given the updated version of modeltests)

@softloud
Copy link
Contributor Author

softloud commented Apr 8, 2019

Wow, @malcolmbarrett - this is amazing! Taking a look to see what I can do now.

R/rma-tidiers.R Outdated Show resolved Hide resolved
R/rma-tidiers.R Outdated
#' @templateVar class rma
#' @template title_desc_tidy
#'
#' @param x An `rma` created by the `metafor` package.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would link to all the function that create rma objects here. That is, something along the lines of:

#' @param x An rma such as those created by [metafor::function1()], [metafor::function2()] or ....

R/rma-tidiers.R Outdated
}

# remove extra model data from study names
attributes(results$study) <- NULL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, metafor stores a lot of extra information in these vectors which caused problems in working with data frames, but I suspect being stricter while assembling tibbles and using dplyr::bind_*() already stripped these and now it's no longer necessary.

@alexpghayes
Copy link
Collaborator

This is looking fantastic. Left three more short comments.

alexpghayes and others added 3 commits Apr 8, 2019
Co-Authored-By: softloud <charlestigray@gmail.com>
fix a couple bugs, tweak docs, remove attribute stripping
@malcolmbarrett
Copy link
Contributor

Thanks, @alexpghayes and @softloud. Addressed!

@alexpghayes alexpghayes merged commit 8e542fe into tidymodels:master Apr 8, 2019
@alexpghayes
Copy link
Collaborator

Fantastic work! Thrilled to have this in broom, thank you so much!

@malcolmbarrett
Copy link
Contributor

Yess, thanks for all your help, @alexpghayes!

@github-actions
Copy link

github-actions bot commented Mar 8, 2021

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 8, 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

3 participants