DeclareDesign / DesignLibrary

Library of Research Designs
https://declaredesign.org/library/
Other
30 stars 3 forks source link

new names and aliases #213

Closed macartan closed 5 years ago

macartan commented 5 years ago

why do these have different behavior? shouldn't simple_two_arm_designer now be an alias for two_arm_designer?

> design_2 <- two_arm_designer(N = 100, ate = -.2)
> design_2 <- simple_two_arm_designer(N = 100, ate = -.2)
Error in eval(parse(text = paste(arg_names[j], " <- ", args[arg_names[j]]))) : 
  ..2 used in an incorrect context, no ... to look in
In addition: Warning message:
'simple_two_arm_designer' is deprecated.
Use 'two_arm_designer' instead.
See help("Deprecated") 
jaspercooper commented 5 years ago

Interesting, sorry about that. I'll look into a solution.

jaspercooper commented 5 years ago

OK, the problem lies with how the substitution approach used to convert "arguments into values" in the code printing is interacting with the method used to alias the old functions.

Here's how the alias works:

simple_two_arm_designer <- function(...){
  .Deprecated("two_arm_designer")
  two_arm_designer(...)
}

In construct_design_code() you have the following code that substitutes in values for symbolic argument names in the assignments at the top when arguments_as_values = TRUE:

if(arguments_as_values){
    # Evaluate args in order provided in formals
    for(j in 1:length(arg_names)) eval(parse(text = paste(arg_names[j], " <- ", args[arg_names[j]])))  
    arg_vals <- sapply(arg_names, function(x) eval(parse(text = paste0("c(", paste(x, collapse = ","), ")"))))
    # convert args to text
    args_text <- paste(sapply(arg_names, function(x) paste(x, " <- ", arg_vals[x])))
  } else {
    # convert args to text
    args_text <- as.character(sapply(names(args[2:length(args)]), function(x) paste0(x, " <- ", deparse(args[[x]]))))
  }

Even if you use a different aliasing approach like this


simple_two_arm_designer <- function(N = 100,
                                    assignment_prob = .5,
                                    control_mean = 0,
                                    control_sd = 1,
                                    ate = 1,
                                    treatment_mean = control_mean + ate,
                                    treatment_sd = control_sd,
                                    rho = 1){
  .Deprecated("two_arm_designer")

  two_arm_designer(N = N,
                   assignment_prob = assignment_prob,
                   control_mean = control_mean,
                   control_sd = control_sd,
                   ate = ate,
                   treatment_mean = treatment_mean,
                   treatment_sd = treatment_sd,
                   rho = rho)
}

You're going to run into the same problem: the eval / parse business doesn't know where to evaluate the arguments.

A few solutions, none of which is really perfect:

  1. We alias like this simple_two_arm_designer <- two_arm_designer. Con: no deprecate message.
  2. We fully duplicate the function, including the whole body but also a deprecate message in the simple_ versions. Con: maintaining two versions of the same code.
  3. We turn off arguments_as_values. Con: loss of functionality.

Others probably have better ideas.

nfultz commented 5 years ago

You should probably refactor that using rlang quosures, they let you more finely control the context of the evaluation.

On Fri, Nov 23, 2018 at 9:53 AM Jasper Cooper notifications@github.com wrote:

OK, the problem lies with how the substitution approach used to convert "arguments into values" in the code printing is interacting with the method used to alias the old functions.

Here's how the alias works:

simple_two_arm_designer <- function(...){ .Deprecated("two_arm_designer") two_arm_designer(...) }

In construct_design_code() you have the following code that substitutes in values for symbolic argument names in the assignments at the top when arguments_as_values = TRUE:

if(arguments_as_values){

Evaluate args in order provided in formals

for(j in 1:length(arg_names)) eval(parse(text = paste(arg_names[j], " <- ", args[arg_names[j]])))
arg_vals <- sapply(arg_names, function(x) eval(parse(text = paste0("c(", paste(x, collapse = ","), ")"))))
# convert args to text
args_text <- paste(sapply(arg_names, function(x) paste(x, " <- ", arg_vals[x])))

} else {

convert args to text

args_text <- as.character(sapply(names(args[2:length(args)]), function(x) paste0(x, " <- ", deparse(args[[x]]))))

}

Even if you use a different aliasing approach like this

simple_two_arm_designer <- function(N = 100, assignment_prob = .5, control_mean = 0, control_sd = 1, ate = 1, treatment_mean = control_mean + ate, treatment_sd = control_sd, rho = 1){ .Deprecated("two_arm_designer")

two_arm_designer(N = N, assignment_prob = assignment_prob, control_mean = control_mean, control_sd = control_sd, ate = ate, treatment_mean = treatment_mean, treatment_sd = treatment_sd, rho = rho) }

You're going to run into the same problem: the eval / parse business doesn't know where to evaluate the arguments.

A few solutions, none of which is really perfect:

  1. We alias like this simple_two_arm_designer <- two_arm_designer. Con: no deprecate message.
  2. We fully duplicate the function, including the whole body but also a deprecate message in the simple_ versions. Con: maintaining two versions of the same code.
  3. We turn off arguments_as_values. Con: loss of functionality.

Others probably have better ideas.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/DeclareDesign/DesignLibrary/issues/213#issuecomment-441295273, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZjTiPgus727KoPkyP_kCf-1FESPRY5ks5uyDYQgaJpZM4YvIsR .

jaspercooper commented 5 years ago

OK thanks, I'll look into this

clarabicalho commented 5 years ago

This works and keeps functionality with default values, but is there a more elegant approach?

simple_two_arm_designer <- function(N = 100,
                                    assignment_prob = .5,
                                    control_mean = 0,
                                    control_sd = 1,
                                    ate = 1,
                                    treatment_mean = control_mean + ate,
                                    treatment_sd = control_sd,
                                    rho = 1){
  .Deprecated("two_arm_designer")
  rlang::eval_bare(rlang::expr(two_arm_designer(N = !!N,
                                         assignment_prob = !!assignment_prob,
                                         control_mean = !!control_mean,
                                         control_sd = !!control_sd,
                                         ate = !!ate,
                                         treatment_mean = !!treatment_mean,
                                         treatment_sd = !!treatment_sd,
                                         rho = !!rho)))
}
jaspercooper commented 5 years ago

thanks @clabima -- this is a great temporary solution to the issue (we should importfrom those rlang functions directly, though).

Eventually, we will want to refactor the guts of construct_design_code using something very similar to this so that we've got a more general solution, don't you think?

clarabicalho commented 5 years ago

My understanding is the issue is with the language returned by match.call.defaults(), which uses the values set in the same frame level regardless of whether you're using two_arm_designer() or simple_two_arm_designer <- function(…) two_arm_designer(...). I couldn't come up with a solution in the helper functions that would seamlessly take account of different frame stacks. But following on Neal's pointer, I refactored the call so that it evaluates within the right frame, and slightly less messy than earlier suggestion:

simple_two_arm_designer <- function(...){
  .Deprecated("two_arm_designer")
  dots <- list2(...)
  eval_bare(expr(two_arm_designer(!!!dots)))
}
jaspercooper commented 5 years ago

OK that is awesome, thank you so much