datacamp / testwhat

Write Submission Correctness Tests for R exercises
https://datacamp.github.io/testwhat
GNU Affero General Public License v3.0
33 stars 25 forks source link

check_arg("formula") not robust for models that are S3 objects #223

Closed amy-datacamp closed 5 years ago

amy-datacamp commented 5 years ago

Related to #222

Feedback message on 2019-01-06:

gee_mod_exch <- geeglm(symptom ~ 1 + age + sex + month, data = madras, id = id, corstr = "exchangeable", family = binomial, scale.fix = TRUE)

In examples like Exercise 4.9 in Longitudinal Analysis that create a model that is an S3 object, check_arg("formula") flags an answer as incorrect when the formula is submitted in a different order than the answer. check_code needs to be submitted to accept answers that are correct but have the formula in a different order than the solution.

hermansje commented 5 years ago

What is the function you expect to accept different orderings?

amy-datacamp commented 5 years ago

The function for these exercises is geeglm().

hermansje commented 5 years ago

I was aiming at the testwhat function after check_arg 🙂

amy-datacamp commented 5 years ago

Teach link to exercise

Here is the SCT:

ex() %>% check_correct({
    check_or(
        ., 
        check_code(., "geeglm(symptom ~ 1 + month + age + sex, id = id, data = madras, family = binomial, corstr = 'exchangeable', scale.fix = TRUE)", missing_msg = "Did you define the GEE model correctly?", fixed = TRUE, append = FALSE),
        check_code(., "geeglm(symptom ~ 1 + age + month + sex, id = id, data = madras, family = binomial, corstr = 'exchangeable', scale.fix = TRUE)", missing_msg = "Did you define the GEE model correctly?", fixed = TRUE, append = FALSE),
        check_code(., "geeglm(symptom ~ 1 + age + sex + month, id = id, data = madras, family = binomial, corstr = 'exchangeable', scale.fix = TRUE)", missing_msg = "Did you define the GEE model correctly?", fixed = TRUE, append = FALSE)
    )
}, 
  {
  check_object(., "gee_mod_exch") %>% check_equal() 
  check_function(., "geeglm") %>% {
    check_arg(., "formula") %>% check_equal(eval = FALSE) 
    check_arg(., "id") %>% check_equal()
    check_arg(., "family") %>% check_equal()
    check_arg(., "data") %>% check_equal(eval = FALSE)
    check_arg(., "corstr") %>% check_equal()
    check_arg(., "scale.fix") %>% check_equal()
  } 
 }
)
hermansje commented 5 years ago

The check_object should be the first argument of the check_correct. The check_or should come after check_function in the second argument of the check_correct. The check_code now checks too much, and it should use a regex instead of fixed matching to allow for variable spacing.

Although it's not ideal that SCTs have to be written this way currently, I think everything works as intended and the above improvements will result in the wanted exercise feedback.

amy-datacamp commented 5 years ago

Hi @hermansje,

For exercises that create models that are S3 objects, using the SCT below allows the formula arguments to be in a different order without including check_code.

ex() %>% check_correct({
  check_expr(., "sort(gee_mod_exch$coefficients)") %>% check_result() %>% check_equal(incorrect_msg = "Did you define the GEE model correctly?", append = FALSE)
}, {
  check_function(., "geeglm") %>% {
    check_arg(., "formula")
    check_arg(., "id") %>% check_equal()
    check_arg(., "family") %>% check_equal()
    check_arg(., "data") %>% check_equal(eval = FALSE)
    check_arg(., "corstr") %>% check_equal()
    check_arg(., "scale.fix") %>% check_equal()
  }
})
richierocks commented 5 years ago

I realize that this is closed, but I'm now responsible for this course as well as a few others where models are being tested and it's possible to specify formulae in different ways. So I thought I'd document my latest thinking.

Some principles:

  1. It's good to have different feedback depending on whether the student got the left or right hand side of the formula wrong.
  2. The code ought to be generalizable across many model types.

Some techniques I make use of here:

  1. Many models include a copy of the formula somewhere in the object. It moves around a lot, but terms() is supposed to retrieve it if it exists.
  2. Similarly, the coefficients move around from model to model, but coefficients() should always retrieve them.
  3. rlang contains f_lhs() and f_rhs() for extracting the left and right hand side of a formula. Usually there is only one way of specifying the left hand side, so we are OK to use f_lhs().

Here's my modified version of @amy-datacamp's code.

ex() %>% check_correct({
  check_object(., "gee_mod_exch")
  check_expr(., "rlang::f_lhs(terms(gee_mod_exch))") %>% 
    check_result() %>% 
    check_equal(incorrect_msg = "Did you specify `symptom` as the response variable in the GEE model?")
  check_expr(., "sort(coefficients(gee_mod_exch))") %>% 
    check_result() %>% 
    check_equal(incorrect_msg = "Did you specify an intercept, `age`, `month`, and `sex` as explanatory variables in the GEE model?")
}, {
  check_function(., "geeglm") %>% {
    check_arg(., "formula")
    check_arg(., "id") %>% check_equal()
    check_arg(., "family") %>% check_equal()
    check_arg(., "data") %>% check_equal(eval = FALSE)
    check_arg(., "corstr") %>% check_equal()
    check_arg(., "scale.fix") %>% check_equal()
  }
})