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

Selecting function updates #685

Merged
merged 23 commits into from
Nov 11, 2020
Merged

Selecting function updates #685

merged 23 commits into from
Nov 11, 2020

Conversation

ddsjoberg
Copy link
Owner

@ddsjoberg ddsjoberg commented Oct 21, 2020

What changes are proposed in this pull request?

  • All select helper functions and the utility functions that make them possible, have been cleaned up and migrated to broom.helpers. THIS IS A HUGE UPDATE.
  • Migrated all selecting functionality from gtsummary to broom.helpers. Exporting functions the functions below. Each has been improved and streamlined compared to their original versions in gtsummary.
    • .generic_selector(): this is a function that makes it easy to create selecting functions like all_continuous(). The internals allow for it to be used in broom.helpers and gtsummary seamlessly.
    • .select_to_varnames(): This function converts selecting syntax into character varnames
    • .formula_list_to_named_list(): this function takes the formula selecting syntax used widely in gtsummary, and converts it to a named list.
  • Update to use broom.helpers::tidy_plus_plus() instead of the individual broom.helpers::tidy_*() functions.
  • Theme element has been added for controlling the other tidy_plus_plus() arguments.
  • tbl_regression(add_estimate_to_reference_rows=) argument has been added. Added to tbl_uvregression() as well.
  • Theme element for tbl_regression(add_estimate_to_reference_rows=) has been added.
  • The argument all_continuous(continuous2=) has been removed. No deprecation messages were added...it was just cut.

If there is an GitHub issue associated with this pull request, please provide link.
closes #648
closes #680
closes #692
closes #677


Checklist for PR reviewer

  • PR branch has pulled the most recent updates from master branch. Ensure the pull request branch and your local version match and both have the latest updates from the master branch.
  • If an update was made to tbl_summary(), was the same change implemented for tbl_svysummary()?
  • If a new function was added, function included in _pkgdown.yml
  • If a bug was fixed, a unit test was added for the bug check
  • Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
  • Code coverage is suitable for any new functions/features. Review coverage with covr::report(). Before you run, set Sys.setenv(NOT_CRAN=true) and begin in a fresh R session without any packages loaded.
  • R CMD Check runs without errors, warnings, and notes
  • NEWS.md has been updated with the changes from this pull request under the heading "# gtsummary (development version)". If there is an issue associated with the pull request, reference it in parantheses at the end update (see NEWS.md for examples).
  • usethis::use_spell_check() runs with no spelling errors in documentation
  • When the branch is ready to be merged into master, increment the version number using usethis::use_version(which = "dev"), run codemetar::write_codemeta(), approve, and merge the PR.

@ddsjoberg ddsjoberg marked this pull request as ready for review October 29, 2020 05:15
@ddsjoberg ddsjoberg changed the title Use tidy_plus_plus() Selecting function updates Oct 30, 2020
@ddsjoberg
Copy link
Owner Author

@karissawhiting @michaelcurry1123 helllllooo! I miss you guys!

Can either of you or both of you review this PR?

@ddsjoberg
Copy link
Owner Author

@karissawhiting @michaelcurry1123

To prepare the tbl_regression() .$table_body we are now using broom.helpers::tidy_plus_plus() https://larmarange.github.io/broom.helpers/reference/tidy_plus_plus.html
It has all the functionality of to prepare the table body we had already implemented plus other options. All the arguments in tidy_plus_plus() not already utilized in tbl_regression() can by modified with theme elements. But what do you think of adding some/all of these options as arguments to tbl_regression().

  1. I think the most widely used arg may be add_estimate_to_reference_rows=, which will add a 0 (or 1 of exponentiated) to the reference row (we currently show an emdash). We could add just this one argument.

  2. We could add an argument like tbl_regression(tidy_plus_plus = list(add_estimate_to_reference_rows = TRUE, other.arg = FALSE)), where users could pass a list of arguments that will then be passed to tidy_plus_plus().

  3. We could add both tbl_regression(add_estimate_to_reference_rows=, tidy_plus_plus=)?

  4. We could add none of them, and keep control of these elements via themes only.

What do you think? ❤️

@larmarange
Copy link
Collaborator

From my point of view, I would prefer solution 1 or 3.

@ddsjoberg ddsjoberg mentioned this pull request Nov 1, 2020
10 tasks
@michaelcurry1123
Copy link
Contributor

@ddsjoberg I would agree with the above, I think 1 and/or 3 would be ideal.

@ddsjoberg ddsjoberg mentioned this pull request Nov 1, 2020
10 tasks
@ddsjoberg
Copy link
Owner Author

Thanks @larmarange @michaelcurry1123 , i'll implement number 1 for now, and if we choose we can always add number 3 before the release, or whenever

@michaelcurry1123
Copy link
Contributor

@ddsjoberg should I update the branch?

@ddsjoberg
Copy link
Owner Author

ddsjoberg commented Nov 2, 2020

@michaelcurry1123 Update with corrections? Sure!

@ddsjoberg
Copy link
Owner Author

Ohh updating from master 👍👍👍

@michaelcurry1123
Copy link
Contributor

@ddsjoberg I think you had to review, apologize if you did not want the update from master

@ddsjoberg
Copy link
Owner Author

Oh updating was def the thing to do @michaelcurry1123

@michaelcurry1123
Copy link
Contributor

michaelcurry1123 commented Nov 3, 2020

There is some masking issues with the migration of the select helpers to broom.helpers
The first example below will show the iqr and range. If you load broom.helpers after gtsummary the helper functions are masked and it does not display the other metric (range). The third example is a work around but not sure if you want that to be the way this selectors work. @ddsjoberg

library(gtsummary)

#this table will give you the median iqr and range

tbl_summary(select(trial,age,trt),
            by = trt,
            type = all_continuous() ~ "continuous2",
            statistic = all_continuous() ~ c(
                          "{median} ({p25}, {p75})", 
                          "{min}, {max}" ))

#restart r here

library(gtsummary)
library(tidyverse)
library(broom.helpers)

#no range is displayed
tbl_summary(select(trial,age,trt),
            by = trt,
            type = all_continuous() ~ "continuous2",
            statistic = all_continuous() ~ c(
              "{median} ({p25}, {p75})", 
              "{min}, {max}" ))


tbl_summary(select(trial,age,trt),
            by = trt,
            type = gtsummary::all_continuous() ~ "continuous2",
            statistic = gtsummary::all_continuous() ~ c(
              "{median} ({p25}, {p75})", 
              "{min}, {max}" ))
#edit the code below works but I am not sure why, that is all_continuous() from broom.helpers and not gtsummary

tbl_summary(select(trial,age,trt),
            by = trt,
            type = all_continuous() ~ "continuous2",
            statistic = all_continuous2() ~ c(
              "{median} ({p25}, {p75})", 
              "{min}, {max}" ))


@ddsjoberg
Copy link
Owner Author

@michaelcurry1123 ! thanks!

this issue comes up because both gtsummary and broom.helpers export a all_continuous() function and they are slightly different. The gtsummary version captures both "continuous" and "continuous2" summary types. The "continuous2" type does not exist in broom.helpers and broom.helpers::all_continuous() only captures type "continuous".

There are a couple of ways we could tackle this:

  1. Do nothing, and assume that most users won't be using both gtsummary and broom.helpers at the same time.

  2. The gtsummary::all_continuous(continuous2 = TRUE) has an argument that selects continuous2 types. We could make an update to broom.helpers::all_continuous() to select all types that start with "continuous", thus selecting both continuous types. We could then import broom.helpers::all_continuous() into gtsummary as is with no masking conflict. This would be a breaking change for gtsummary::all_continuous() because users would no longer be able to select type continuous, excluding type continuous2 (but I don't think anyone is really doing that). If we make this change, every all_*() function in broom.helpers and gtsummary would be identical and they could all be imported from broom.helpers. @larmarange would this update work for you on the broom.helpers side?

@larmarange
Copy link
Collaborator

2. @larmarange would this update work for you on the broom.helpers side?

Not a problem on my side. We cant update broom.helpers accordingly

@michaelcurry1123
Copy link
Contributor

Would it be safe to assume people would not use these packages at the same time? @larmarange did you mean you could update or cannot update? In your above response it says can't and not can

@larmarange
Copy link
Collaborator

Sorry. Can.

@ddsjoberg
Copy link
Owner Author

Thanks @michaelcurry1123 and @larmarange for the input

  1. I updated broom.helpers::all_continuous() to select all types that begin with "continuous"; therefore, selecting "continuous2" as well. This is a breaking change for gtsummary::all_continuous(continuous2 = TRUE) as there is no more function argument. Pending PR Update to all_continuous() larmarange/broom.helpers#75

  2. All the select helpers in broom.helpers are now imported and re-exported by gtsummary (no more name conflicts).

  3. An unrelated change made is that the all_factor(), all_character() family of functions are warn deprecated instead of defunct, making them still usable until their final deprecation.

@ddsjoberg
Copy link
Owner Author

Hello @michaelcurry1123 ! Just wanted to confirm whether this review is done? 🍁

@michaelcurry1123 michaelcurry1123 merged commit 2d19324 into master Nov 11, 2020
@ddsjoberg ddsjoberg deleted the tidy_plus_plus branch November 11, 2020 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants