search

kgoldfeld / simstudy

simstudy: Illuminating research methods through data generation
https://kgoldfeld.github.io/simstudy/
GNU General Public License v3.0
81 stars 7 forks source link

216 nonrandom distribution returns a single value when repeated values are expected #225

Closed kgoldfeld closed 5 months ago

kgoldfeld commented 6 months ago

@assignUser I tried to create a test for the new custom functionality, and of course, I need to create a function as part of the test. However, the test doesn't run, because it can't see the function for some reason. Here is my test - any thoughts on how I can make it work?

test_that("custom data is generated as expected.", {
  skip_on_cran()

  trunc_norm <- function(n, lower, upper, mu = 0, s = 1.5) {

    F.a <- pnorm(lower, mean = mu, sd = s)
    F.b <- pnorm(upper, mean = mu, sd = s)

    u <- runif(n, min = F.a, max = F.b)
    qnorm(u, mean = mu, sd = s)

  }

  def <-
    defData(varname = "x", formula = 5, dist = "poisson") |>
    defData(varname = "y", formula = ".trunc_norm",
            variance = "s = 100, lower = x - 1, upper = x + 1",
            dist = "custom"
    )

  dd <- genData(10000, def)

  expect_true( dd[, min(y)] > dd[, min(x-1)])
  expect_true( dd[, max(y)] < dd[, max(x+1)])

})

Here is the error message:

dim = c(10000L, 
1L), dimnames = list(NULL, "x+1")))`: could not find function "trunc_norm"
Backtrace:
    ▆
 1. └─simstudy::genData(10000, def) at test-generate_dist.R:195:3
 2.   └─simstudy:::.generate(...) at simstudy/R/generate_data.R:86:7
 3.     └─simstudy:::.gencustom(...) at simstudy/R/generate_dist.R:16:3
 4.       └─base::do.call(fn, arg_list) at simstudy/R/generate_dist.R:371:3
[ FAIL 1 | WARN 1 | SKIP 0 | PASS 28 ]

Test complete

Of course, the code works if I just run it outside the context of a test.

github-actions[bot] commented 6 months ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if b6919ccd23a88714dd5a9aa2903283b37d006563 is merged into main:

kgoldfeld commented 6 months ago

I figured out the test issue - needed to change the environment when in the do.call(). It has the added benefit of working if the custom function is defined within another function.

I've created a new vignette - that should give you a better idea of how it is all working.

Let me know if you think I can merge this.

github-actions[bot] commented 6 months ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 6c10b214cfcb782166ec814fa17933580fe24181 is merged into main:

github-actions[bot] commented 5 months ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 4bf87a0bd7a02503b9441961a10e2e1ef8dd720b is merged into main:

kgoldfeld commented 5 months ago

Wow, very cool, nice job! Neat how the systems come together and something is easier than expected 😁

I have just two small question:

* this will at the moment only work with arguments fully specified as `name = vale||var` (just from looking at the code) If I am not mistaken I think that should be mentioned in the documentation to avoid confusion. (or the code changed to accept a mix as well but I think it's totally fine keeping it as is)

Yes - I will make it clear that we need that format "name = value or formula"

* double-dot variables should also work right? that might also be worth a mention.

Yes - double dot works here as well - that was the beauty of the system you set up - I was able to take advantage here.

kgoldfeld commented 5 months ago

I have updated the help documentation in simstudy.Rmd.

One thought I had. I now have the function name in the defData formula field, and the formula string in the variance field, becuase I thought it was logical to have the function name first. However, it might be less confusing to switch them so that the formula string is in the defData formula field, and the function name in the defData variance field. Do you think the change would be useful?

github-actions[bot] commented 5 months ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if f2970243e400a84d072794a45ff483618b7f6524 is merged into main:

github-actions[bot] commented 5 months ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 57a593963159e63a936628491d2353b373b65870 is merged into main:

assignUser commented 5 months ago

Do you think the change would be useful?

I think either way works but I would also tend to put the function name first, it just feels 'natural' even if the field name doesn't fit ^^

kgoldfeld commented 5 months ago

I guess I landed in the same place as you. This one is in the books - since it has been merged. I will likely be submitting the current development version as simstudy 0.8.0 in the next few weeks.