metafor::rma tidiers - work in progress #674

softloud · 2019-04-04T11:30:26Z

These tidiers are still work in progress with @malcolmbarrett.

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

resources

broom:: helpfully has pointers on what to do for a pr
broom:: test examples
10 steps to being a tidyverse contributor
modeltests documentation
this advice from @alexpghayes
the original thread discussing the tests

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

Add tests

softloud · 2019-04-06T07:10:10Z

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?

fix test errors, temporarily set strict = FALSE

add rma to news, mb orcid

fix cmd check errors, broom inconsistencies, set strict = TRUE

alexpghayes

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!

alexpghayes · 2019-04-07T18:54:04Z

R/rma-tidiers.R

 #'    output?
-#' @param ... additional arguments
-#' @param measure measure type. See [metafor::rma()]
+#' @param ... Additional arguments


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.

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)?

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

NEWS.md

alexpghayes · 2019-04-07T18:55:58Z

R/rma-tidiers.R

@@ -0,0 +1,222 @@
+#' Tidying methods for meta-analyis objects


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.

alexpghayes · 2019-04-07T19:00:45Z

R/rma-tidiers.R

+#' @param ... Additional arguments
+#' @param measure Measure type. See [metafor::rma()]
+#'
+#' @return A `tibble`


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.

alexpghayes · 2019-04-07T19:01:09Z

R/rma-tidiers.R

+#'
+#' tidy(meta_analysis)
+#'
+#' @rdname tidiers


Suggested change

#' @rdname tidiers

#' @rdname metafor_tidiers

alexpghayes · 2019-04-07T19:05:35Z

R/rma-tidiers.R

+    purrr::discard(~length(.x) >= 2) %>%
+    # change to tibble with correct column and row names
+    as.data.frame() %>%
+    tibble::as_tibble() %>%


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

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

R/rma-tidiers.R

alexpghayes · 2019-04-07T19:07:08Z

R/rma-tidiers.R

+  ret <- tibble::as_tibble(ret) %>%
+    tibble::remove_rownames()
+
+  if (all(ret$.rownames == seq_along(ret$.rownames))) {


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

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

alexpghayes · 2019-04-07T19:08:44Z

tests/testthat/test-rma.R

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


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

alexpghayes · 2019-04-07T19:09:23Z

tests/testthat/test-rma.R

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


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

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

Or should we use modeltests::check_augment_function?

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

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.

augment args to unstrict

fix templating, testing, binding, rownames

change col names: q* -> cochran.q*, del -> loo

malcolmbarrett · 2019-04-08T00:44:31Z

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

softloud · 2019-04-08T00:50:59Z

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

R/rma-tidiers.R

alexpghayes · 2019-04-08T00:47:30Z

R/rma-tidiers.R

+#' @templateVar class rma
+#' @template title_desc_tidy
+#' 
+#' @param x An `rma` created by the `metafor` package.


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 ....

alexpghayes · 2019-04-08T00:49:07Z

R/rma-tidiers.R

+  }
+
+  # remove extra model data from study names
+  attributes(results$study) <- NULL


Why is this necessary?

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 · 2019-04-08T00:51:48Z

This is looking fantastic. Left three more short comments.

Co-Authored-By: softloud <charlestigray@gmail.com>

fix a couple bugs, tweak docs, remove attribute stripping

malcolmbarrett · 2019-04-08T01:47:43Z

Thanks, @alexpghayes and @softloud. Addressed!

alexpghayes · 2019-04-08T02:11:17Z

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

malcolmbarrett · 2019-04-08T02:12:45Z

Yess, thanks for all your help, @alexpghayes!

github-actions · 2021-03-08T00:26:42Z

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.

softloud added 4 commits April 4, 2019 22:17

ported @malcolmbarrett's testing script

ea0234f

added modeltests 🎶

77cd17b

Merge pull request #1 from softloud/add-tests

b85e01d

Add tests

basic tests added 🎶

725c33a

softloud changed the title ~~ported @malcolmbarrett's testing script~~ metafor::rma tidiers Apr 6, 2019

softloud changed the title ~~metafor::rma tidiers~~ metafor::rma tidiers - work in progress Apr 6, 2019

malcolmbarrett and others added 4 commits April 6, 2019 16:18

fix test errors, temporarily set strict = FALSE

219d932

Merge pull request #2 from softloud/tweak-tests

7078e2b

fix test errors, temporarily set strict = FALSE

added @malcolmbarrett and I as ctb 🎶

0b70c4a

fix cmd check errors, broom inconsistencies, set strict = TRUE

55ed153

This was referenced Apr 7, 2019

add arguments and columns related to rma tidiers alexpghayes/modeltests#20

Merged

add no_data argument to augment arg check alexpghayes/modeltests#21

Closed

malcolmbarrett and others added 6 commits April 7, 2019 09:59

add rma to news, mb orcid

a4ffcf5

rebuild docs

1b69212

Merge pull request #4 from softloud/add-mb-contr

0a7c4f6

add rma to news, mb orcid

Merge branch 'add-tests' into change-examples

26d8d63

Merge pull request #3 from softloud/change-examples

273dfc1

fix cmd check errors, broom inconsistencies, set strict = TRUE

augment args to unstrict

53cd56c

alexpghayes requested changes Apr 7, 2019

View reviewed changes

malcolmbarrett and others added 8 commits April 7, 2019 13:02

change col names: q* -> cochran.q*, del -> loo

d68710e

fix templating, testing, binding, rownames

3bcf4fb

fix augment.rma tests and bugs, change to templates

b6d23b5

switch to return*() for docs

5b4e6d7

add comments and rearrange tidy.rma

9f31411

Merge pull request #5 from softloud/add-unstrict-aug

15d50ab

augment args to unstrict

Merge pull request #7 from softloud/alex-changes

9218953

fix templating, testing, binding, rownames

Merge pull request #6 from softloud/tweak-broom-names

2ba35ff

change col names: q* -> cochran.q*, del -> loo

Merge branch 'master' into add-tests

fdd77be

alexpghayes requested changes Apr 8, 2019

View reviewed changes

alexpghayes and others added 3 commits April 8, 2019 11:39

Update R/rma-tidiers.R

d857e57

Co-Authored-By: softloud <charlestigray@gmail.com>

fix a couple bugs, tweak docs, remove attribute stripping

d5834cb

Merge pull request #8 from softloud/last-sweep

5f896bb

fix a couple bugs, tweak docs, remove attribute stripping

alexpghayes merged commit 8e542fe into tidymodels:master Apr 8, 2019

alexpghayes mentioned this pull request Apr 9, 2019

Tidiers for rma objects from the metafor package #621

Closed

github-actions bot locked and limited conversation to collaborators Mar 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metafor::rma tidiers - work in progress #674

metafor::rma tidiers - work in progress #674

softloud commented Apr 4, 2019 •

edited

softloud commented Apr 6, 2019 •

edited

alexpghayes left a comment •

edited

alexpghayes Apr 7, 2019

malcolmbarrett Apr 7, 2019

alexpghayes Apr 7, 2019

alexpghayes Apr 7, 2019

alexpghayes Apr 7, 2019

alexpghayes Apr 7, 2019

alexpghayes Apr 7, 2019

malcolmbarrett Apr 7, 2019

alexpghayes Apr 7, 2019

malcolmbarrett Apr 7, 2019

alexpghayes Apr 7, 2019

alexpghayes Apr 7, 2019

malcolmbarrett Apr 7, 2019

malcolmbarrett Apr 7, 2019

malcolmbarrett Apr 7, 2019

alexpghayes Apr 7, 2019

malcolmbarrett commented Apr 8, 2019 •

edited

softloud commented Apr 8, 2019

alexpghayes Apr 8, 2019

alexpghayes Apr 8, 2019

malcolmbarrett Apr 8, 2019

alexpghayes commented Apr 8, 2019

malcolmbarrett commented Apr 8, 2019

alexpghayes commented Apr 8, 2019

malcolmbarrett commented Apr 8, 2019

github-actions bot commented Mar 8, 2021

		@@ -0,0 +1,222 @@
		#' Tidying methods for meta-analyis objects

metafor::rma tidiers - work in progress #674

metafor::rma tidiers - work in progress #674

Conversation

softloud commented Apr 4, 2019 • edited

resources

tasks

softloud commented Apr 6, 2019 • edited

alexpghayes left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malcolmbarrett commented Apr 8, 2019 • edited

softloud commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexpghayes commented Apr 8, 2019

malcolmbarrett commented Apr 8, 2019

alexpghayes commented Apr 8, 2019

malcolmbarrett commented Apr 8, 2019

github-actions bot commented Mar 8, 2021

softloud commented Apr 4, 2019 •

edited

softloud commented Apr 6, 2019 •

edited

alexpghayes left a comment •

edited

malcolmbarrett commented Apr 8, 2019 •

edited