Closes #2012_derive_vars_dy

[ynsec37]

Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.

Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the devel branch until you have checked off each task.

• Place Closes Bug: derive_vars_dy() return error when input dataset with columns end with "temp" #2012 into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
• Code is formatted according to the tidyverse style guide. Run styler::style_file() to style R and Rmd files
• Updated relevant unit tests or have written new unit tests, which should consider realistic data scenarios and edge cases, e.g. empty datasets, errors, boundary cases etc. - See Unit Test Guide
• If you removed/replaced any function and/or function parameters, did you fully follow the deprecation guidance?
• Update to all relevant roxygen headers and examples, including keywords and families. Refer to the categorization of functions to tag appropriate keyword/family.
• Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
• Address any updates needed for vignettes and/or templates
• Update NEWS.md if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers)
• Build admiral site pkgdown::build_site() and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.
• Address or fix all lintr warnings and errors - lintr::lint_package()
• Run R CMD check locally and address all errors and warnings - devtools::check()
• Link the issue in the Development Section on the right hand side.
• Address all merge conflicts and resolve appropriately
• Pat yourself on the back for a job well done! Much love to your accomplishment!

The updates were done by passing the named vector of dy_vars to .names in across() and it will not create the temporary variables end with _temp.

A related test that the input dataset has a variable end with _temp was created and no error occurred.

The updates had no impact on user facing and I ticked the some of check list that may be not applicable.

For pkgdown::build_site(), I got the below errors, but I am not sure if it was related to this updates.

For 'lintr::lint_package()', the following message is from the previous commit, and I am not familiar with lintr and wonder if this is a problem.

[ynsec37]

sorry for missing the reviews and I tried to reopen but not found how to add.

[ddsjoberg]

Thank you @ynsec37 for the PR! I'll review it this week 🚀

[ddsjoberg]

@ynsec37 thank you again for the PR.

I have a question about the syntax you used to correct the issue. In the across(.names) argument, you passed a vector of the new names (which works perfectly).

library(dplyr, warn.conflicts = FALSE)

mtcars[1:4, 1:4] %>%
  mutate(
    across(
      .cols = c(mpg, hp),
      .fns = ~. + 1000,
      .names = "{letters[1:2]}"
    )
  )
#>                 mpg cyl disp  hp      a    b
#> Mazda RX4      21.0   6  160 110 1021.0 1110
#> Mazda RX4 Wag  21.0   6  160 110 1021.0 1110
#> Datsun 710     22.8   4  108  93 1022.8 1093
#> Hornet 4 Drive 21.4   6  258 110 1021.4 1110

I haven't see this use of the .names argument documented within the dplyr package. Here's the description of the .names argument from the help file:

If this syntax isn't officially supported, I am concerned to include it in case support is removed in the future. Perhaps we should update it to something like the example below (unless, of course, this is an officially supported use of the argument and I missed it!).

  dataset %>%
    mutate(
      across(
        .cols = vars2chr(unname(source_vars)),
        .fns = ~ compute_duration(start_date = !!reference_date, end_date = .x),
        .names = "{str_replace_all(.col, '(DT|DTM)$', 'DY')}"
      )
    )

Is the vector input to the .names argument official, and if so, where is that documented? THANK YOU! 🍁

[ynsec37]

Thank you very much for review.

I had created a related issue to confirm with dplyr developer whether this is a supported use case.

I had tried your proposal, this may not work for below example, the DEATHDY

derive_vars_dy(
  datain,
  reference_date = TRTSDT,
  source_vars = exprs(TRTSDT, DEATHDY = DTHDT)
)

[bundfussr]

Maybe we should also create an issue at glue to clarify what a "glue expression" is. Is

> glue::glue('Values: {c("a", "b")}')
Values: a
Values: b

intended usage and behavior?

The suggested code in this PR, the suggested code from @ddsjoberg , and also the implementation of dplyr::across() assume it is correct usage. (.col is a vector. So str_replace_all(.col, '(DT|DTM)$', 'DY') is a vector as well.)

[ddsjoberg]

@bundfussr good points, I agree. I am happy to merge as is. I only have a slight concern that the implementation has a small chance of changing in the future since we haven't seen it documented, and the dplyr maintaners may consider it an "off-label" use of their function. Regardless, if that does happen, we'll receive notice from them ahead of any breaking change.

Shall we just merge it?

[ynsec37]

I think the glue::glue is vectorized like other stringr functions or paste0
For example stringr::str_glue

#' mtcars %>% str_glue_data("{rownames(.)} has {hp} hp")
str_glue <- function(..., .sep = "", .envir = parent.frame()) {
glue::glue(..., .sep = .sep, .envir = .envir)
}

glue::glue('Values: {c("a", "b")}')
#> Values: a
#> Values: b

stringr::str_glue('Values: {c("a", "b")}')
#> Values: a
#> Values: b

identical(
  unclass(glue::glue('Values: {c("a", "b")}')),
  paste0("Values: ", c("a", "b"))
)
#> [1] TRUE


name <- c("Fred", "Tom")
glue::glue("My name is {name}")
#> My name is Fred
#> My name is Tom

# as it is vectorized, so it can be used in the `mutate`
head(iris, 1) |>
  dplyr::mutate(description = glue::glue("This {Species} has a petal length of {Petal.Length}"))
#>   Sepal.Length Sepal.Width Petal.Length Petal.Width Species
#> 1          5.1         3.5          1.4         0.2  setosa
#>                             description
#> 1 This setosa has a petal length of 1.4

^{Created on 2023-07-26 with reprex v2.0.2}

[bundfussr]

@bundfussr good points, I agree. I am happy to merge as is. I only have a slight concern that the implementation has a small chance of changing in the future since we haven't seen it documented, and the dplyr maintaners may consider it an "off-label" use of their function. Regardless, if that does happen, we'll receive notice from them ahead of any breaking change.

Shall we just merge it?

I agree that the documentation of dplyr::across() and glue::glue() is not clear and that there is a small chance that the behavior changes in future. However, the risk seems acceptable to me. So I would merge it.

[bundfussr]

I think the glue::glue is vectorized like other stringr functions or paste0

Yes, it is vectorized. But it is strange that it is not mentioned in the documentation and none of the examples demonstrates that it is vectorized. Should we ask the glue developers to update their documentation?

[bms63]

@ynsec37 many thanks for this!!

I'm a bit hesitant to merge something that might mean a later update - is there not another way to accomplish the same task that introduces less risk?

[bms63]

Can you update the news @ynsec37. We decided this is a go!! tyty

[ynsec37]

The news was updated.

As the usage in this pr code is pending to confirmed by dplyr developer.

I create a new branch #2012_derive_vars_dy_patch2@devel that use another way to set names of dy_vars.
It can pass all test too.

  names(source_vars) <- dy_vars


  dataset %>%
    # coping `source_var` to `dy_vars`
    mutate(!!!source_vars) %>%
    # compute on the `dy_vars`
    mutate(
      across(
        .cols = vars2chr(unname(dy_vars)),
        .fns  = ~ compute_duration(start_date = !!reference_date, end_date = .)
    ))

It first create new columns of dy_vars by coping the value of source_vars , then perform computation on newly created columns of dy_vars.

For instance,

derive_vars_dy(
  datain,
  reference_date = TRTSDTM,
  source_vars = exprs(TRTSDTM, ASTDTM, AENDT)
)

1. create targeted dy_vars first

2. compute on these columns
   

[ddsjoberg]

Thank you @ynsec37 for the pull request! very much appreciated!

[ynsec37]

thanks a lot for review
I learn much from this great package admiral