Suggestion: exprs() vs tidyselect

[ddsjoberg]

Background Information

Hello Team! 🍁 Thanks for all your amazing work on this package!

I know I am late to the conversation about the use of exprs(). But I wanted to make a suggestion.

• One thing that makes R difficult for newer users is the diversity in the styles used. For example, the way we use tidyverse functions is often very different from the way we use base R functions (thinking about unquoted column names primarily).
• The admiral pkg has adopted many of style conventions from the tidyverse, which is great.
• The recent update to use exprs() for the user-facing inputs is a deviation from the tidyverse style of function inputs, and I don't think there is another package that uses this input type.
• I think keeping consistent API will be beneficial to new R users (well, and experienced R users too!).
• My suggestion is to accept tidyselect notation for the column selection rather than using exprs(). So an input like by_vars = exprs(USUBJID, PARAMCD), would then look like any of these (or any other tidyselect-consistent notation):
  • by_vars = c(USUBJID, PARAMCD)
  • by_vars = any_of(c("USUBJID", "PARAMCD"))
  • by_vars = all_of(c("USUBJID", "PARAMCD"))
• To keep the scope of the update small, we could write a small function that accepts the tidyselect notation and converts it to a list of expressions, e.g. as_expr_list(<tidyselectors>). This small function could go at the top of any function that is expecting the exprs() input. The function could also be written to handle inputs that use vars() and exprs().
• I think this will help smooth the transition from SAS to R, because it reduces the types of inputs users will need to memorize and the context where each input type can be used.

Definition of Done

When users can utilize tidyselect notation in the admiral functions

[bundfussr]

@ddsjoberg , thanks for creating this issue. It is not too late because admiral is still experimental and we are working towards a "stable" version.

At the moment, I would not use tidyselect for arguments like by_vars because:

• The tidyselect notation seems to be not stable yet. For example, join_by() was introduced this year and the handling of character vectors has been changed.
• The admiral way of handling arguments differs slightly from tidyverse functions but I think it is simpler to use and more consistent.
  In admiral, arguments which expect a single variable/expression can be specified "as is" without quotation. Arguments which expect more than one variable/expression expect a list of expression (e.g., created by exprs()).
  In dplyr, it depends on the function. For example you can use all_of() in select() but not for the by argument in the *_join() functions.
• tidyselect functions like any_of(), starts_with(), matches() depend on the input data. Especially functions with more than one input dataset like derive_vars_merged() would require more documentation to describe how the tidyselect functions are evaluated. And it would require additional code for this evaluation.
  Accepting tidyselect functions would add more flexibility but I am not sure it is worth the effort.

@pharmaverse/admiral , what do others think?

[bms63]

Hey Daniel. Thank you for the feedback!

I always thought the tidyverse packages were a monolith of stability, but after developing admiral for the last two years I have found this is not the case! It was very eye-opening the constant churn in those packages, which honestly I think is a good sign as they are responding to ever-changing landscape and needs.

I tend to lean towards @bundfussr suggestion that we hold the course with exprs(), but we are still experimental and can always change. Since this is a user-facing issues perhaps we could do some sort of survey on pharmaverse slack to gauge users' interest on the matter (probably wouldn't be till end of Q3). We are aiming for a stable 1.0 release at the beginning of 2024 so we would need to settle this matter by then!

On a side note, we have this admiral_options, which could be of interest.
https://github.com/pharmaverse/admiral/blob/main/R/admiral_options.R

[ddsjoberg]

Thank you both for your thoughtful responses! 🌟

I hear and sympathize with the comments about changes in the tidyverse (particularly in the last ~10 months or so). It's been rough on the devs who chose tidyverse dependencies, and I've shared in that pain with you. When I chose the tidyverse deps, I thought it would simplify my life!

That said, putting the user experience first is important in my view, even when it may make the life of the developer more difficult. Using the ubiquitous tidyselect notation speaks to that, rather than introducing a new kind of variable selection as it requires users to both learn the new selection construct and remember where that construct can and cannot be used (the second part being the more difficult piece for new users).

Anyway, thank you for developing the package, and I fully understand if you choose to close this issue now! 🍁

[manciniedoardo]

I tend to agree with @bms63 - staying with the exprs approach for now, but keeping an eye on the tidyverse way of doing things in case that stabilises in the future. In general though, from a user facing perspective it would be nice in the future to move towards a common "feel" for tidyverse and admiral.

One thing to note is that if we were to commit to this shift at some point in the future, then it would likely be after the release of admiral v1.0.0. For us, 1.0.0. implies a certain sense of stability, so I'd weary of making such a large, breaking change.

[ddsjoberg]

It sounds like generally, there is interested in moving toward a tidyselect framework, and the primary concern is that recent changes to tidyselect potentially signal fragility that would perhaps lead to breaks in the code.

A couple of thoughts for the record to be reviewed at a future time point.

• The recent changes in tidyselect did not break any user-facing code. (Yes, as developers we had to make some changes internally, but users' code all still worked.)
• We can seamlessly incorporate any potential future changes in tidyselect notation immediately in admiral by not parsing of the of tidyselect syntax ourselves. If we instead allow dplyr or tidyselect handle the selection, any changes in tidyselect would be handled by those pkgs.
• I mentioned earlier that only a small change would be needed to accept tidyselect notation. I put together a 2-line function to illustrate. This function could be placed at the top of any function that previously accepted exprs() inputs, to convert the tidyselect syntax to the output of exprs()

tidyselect_to_expr_list <- function(data, cols) {
  # we can add a simple check here to see if user passed `exprs()` or `vars()`.
  # we can print them a message of the correct notation AND then we correct the notation for them, 
  # so it would not break any existing code
  
  # get character vector of selected column names
  cols_chr <- colnames(dplyr::select(data, {{ cols }}))
  
  # convert to list of symbols
  lapply(cols_chr, function(x) rlang::sym(x))
}

# works when there are more than one variable selected
tidyselect_to_expr_list(mtcars, c(am, cyl)) |> str()
#> List of 2
#>  $ : symbol am
#>  $ : symbol cyl
rlang::exprs(am, cyl) |> str()
#> List of 2
#>  $ : symbol am
#>  $ : symbol cyl

# works with a single column as well
tidyselect_to_expr_list(mtcars, cyl) |> str()
#> List of 1
#>  $ : symbol cyl
rlang::exprs(cyl) |> str()
#> List of 1
#>  $ : symbol cyl

# works with tidyselect functions
tidyselect_to_expr_list(mtcars, dplyr::starts_with("d")) |> str()
#> List of 2
#>  $ : symbol disp
#>  $ : symbol drat

^{Created on 2023-05-14 with reprex v2.0.2}

I'll just reiterate that I think adopting tidyselect syntax as it is, without modification, will help with the adoption of admiral package, as opposed to implementing new selecting syntax that looks similar but with a few differences that are unique to the admiral package.

Anyway, thanks for taking the time to go back and forth with me on this!

[thomas-neitmann]

I'm personally quite sympathetic to @ddsjoberg's proposal. I never quite felt like exprs() is a great solution unless it's for parameters like set_values_to() where you actually pass an expression. For simple things like enumerating a number of columns it always looked a bit odd to me.

[bms63]

Hey @ddsjoberg - many thanks for being so engaged and concerned for our users. We are definitely interested in providing a good user experience! I will put onto the agenda for this week's core meeting. @pharmaverse/admiral can everyone come up to speed with this issue so we can discuss some more.

If we do implement, I'm not sure we will do it in this release cycle as we are all chasing things at the moment.

Just fyi: We have been operating in 3 month release cycles. We still see ourselves as experimental package and implementing user feedback has played a big role in each cycle. Hoping that at the end of the year we move to twice or even once a year releases! But still working out the kinks - pkg dev is hard!

[ddsjoberg]

Pkg dev is hard!! Thank you so much for doing it!

[rossfarrugia]

Hey all, sorry late to this discussion, Edoardo mentioned it to me in a call today. Thanks Daniel for sharing ideas for admiral! 👍

My 2 cents would be that I like the suggestion of giving users new and familiar feeling options in a way that doesn't break backward compatibility of their scripts. If this tidyselect suggested approach stabilizes and it helps R users better adapt to our package then I'd be in favour of giving users the option to use it with admiral. To Stefan's point though, we should be mindful of only adding whichever options we feel actually add value to justify the effort - for example I immediately see the benefit of c() through familiarity, but I'm less in favour of things like starts_with() as that just makes user's ADaM scripts that bit more difficult to code review and debug. I rather the user just list the variables clearly.

I also don't think this should be highest on our backlog though, and could come later. If we do go this route, I'd say just cover this option once with an example in the Get Started page, rather than giving examples and these options repeated for every function page or vignette where we show exprs() right now. We could then still keep exprs() as the default for our package and in our templates, but if users prefer they could replace with one of the other options.

@manciniedoardo and @bms63 you'll probably hear a million different viewpoints on this one and there's really no right or wrong, so trust you to make the overall call.

[bundfussr]

Please note that expressions like c(TRTSDT, TRTEDT, TRT01A, TRT01P) only work in the context of functions accepting tidyselect expressions. They can not be used outside. For example

adsl_vars <- c(TRTSDT, TRTEDT, TRT01A, TRT01P)

does not work.

[bms63]

Hey @ddsjoberg heard you are on the admiral team!! Shall we make this happen for our next release (September)?

[ddsjoberg]

Awesome! Let's connect later this week or next to discuss details! (I am getting my tech setup at the moment...hopefully, i'll be up and running soon!)

[zdz2101]

#1990 and pharmaverse/admiraldev#233 on ice until we reached a fully-flushed out conclusion here

[ddsjoberg]

Brief summary of meeting with @bms63 and @manciniedoardo

There is no plan to implement this feature, but may be picked up at some point in the future. Two reasons for not implementing now

1. Limited developer time to make change, and prioritizing other work.
2. Desire to signal to users the package is stable by not adding new feature.

@bms63 you mentioned that you'd like to see a small example how tidyselect could be implemented. In the script linked below there are two primary function:

1. Function to process an argument with tidyselect and return a list of expressions (as is returned by rlang::exprs()). If user passes exprs() the results are returned unaltered.
2. Function to process a merge "by" argument. Similar to above, a list of expressions is returned.

https://github.com/pharmaverse/admiral/blob/tidyselect-example/R/utils-tidyselect.R