Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider alternative implementations for the offspring distribution #167

Closed
jamesmbaazam opened this issue Dec 15, 2023 · 11 comments · Fixed by #188
Closed

Consider alternative implementations for the offspring distribution #167

jamesmbaazam opened this issue Dec 15, 2023 · 11 comments · Fixed by #188
Labels
enhancement New feature or request high-priority
Milestone

Comments

@jamesmbaazam
Copy link
Member

I would remove this in favour of input checking whether roffspring_name() returns count values. Overall I think you should restrict which distributions users can sample from, or allow this to be a parameterised anonymous function as with {cfr}.

Originally posted by @pratikunterwegs in #122 (comment)

@sbfnk
Copy link
Contributor

sbfnk commented Dec 15, 2023

The reason this can't be an anonymous function is that, where analytical solutions are available, we need to look up the corresponding likelihood function (either internally or provided by the user). See also the discussion in #25 and broader discussion in https://github.com/orgs/epiverse-trace/discussions/140

@pratikunterwegs if you had any ideas for how to handle this better it would be great to hear as I don't think the current solution requiring function lookups by name is a particularly satisfactory one.

@pratikunterwegs
Copy link
Collaborator

pratikunterwegs commented Dec 15, 2023

Thanks, I understand the issue/constraints better now. I guess the solution you have is very close to what I would implement; perhaps passing the function rgamma directly instead of a string (for a do.call this doesn't matter much, but can be useful if you want to check for the number of required arguments). IIRC the required arguments are being passed already as .... Restricting the functions to be among those for which you can access a log-likelihood should still work.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 26, 2024

An option here that I hadn't considered is to do the lookup via the substitute function.

I.e. we could change the argument offspring_dist to take a function and then look up it's name, something like

test <- function(offspring_dist) {
  as.character(substitute(offspring_dist))
}

test(rpois)
#> [1] "rpois"
test(stats::rpois)
#> [1] "::"    "stats" "rpois"
test(function (x) { rpois() })
#> [1] "function"                    "as.pairlist(alist(x = ))"   
#> [3] "{\n    rpois()\n}"           "c(7, 6, 7, 29, 6, 29, 7, 7)"

Created on 2024-01-26 with reprex v2.0.2

We could test for the last element of that character vector and if == "rpois" defer to pois_size_ll() and the like. I think this would retain all functionality we have at the moment.

@pratikunterwegs
Copy link
Collaborator

I guess that would work too but could be a little bit more work; alternatively, you could pop a note into the docs saying which functions are supported and break if the provided fn is not among them.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 26, 2024

The key thing is that we need to do a lookup somehow, i.e.

  • user wants Poisson offspring -> we call pois_size_ll()
  • user wants Negativ Binomial offspring -> we call nbinom_size_ll()
  • user wants anything else -> we call offspring_ll()

@pratikunterwegs
Copy link
Collaborator

Using a simple switch()?

fn_ll = switch(distr, rpois = pois_size_ll, nbinom = nbinom_size_ll, offspring_ll)

@sbfnk
Copy link
Contributor

sbfnk commented Jan 26, 2024

Not unless I'm missing something.

fn_ll <- function(distr) switch(distr, rpois = pois_size_ll, nbinom = nbinom_size_ll, offspring_ll)
fn_ll(rpois)
#> Error in switch(distr, rpois = pois_size_ll, nbinom = nbinom_size_ll, : EXPR must be a length 1 vector

Created on 2024-01-26 with reprex v2.0.2

@pratikunterwegs
Copy link
Collaborator

Ah, you'd need to quote the function name after all. Dummy fns for the LL used here.

fn_ll <- function(distr) {
  if (is.function(distr)) {
    distr = as.character(quote(distr))
  }
  switch(distr, rpois = rpois, nbinom = rnbinom, rnorm)
}

fn_ll("rpois")
#> function (n, lambda) 
#> .Call(C_rpois, n, lambda)
#> <bytecode: 0x11ceef9f8>
#> <environment: namespace:stats>
fn_ll(rpois)
#> function (n, mean = 0, sd = 1) 
#> .Call(C_rnorm, n, mean, sd)
#> <bytecode: 0x11e7d2e78>
#> <environment: namespace:stats>
fn_ll(rnbinom)
#> function (n, mean = 0, sd = 1) 
#> .Call(C_rnorm, n, mean, sd)
#> <bytecode: 0x11e7d2e78>
#> <environment: namespace:stats>

Created on 2024-01-26 with reprex v2.0.2

@sbfnk
Copy link
Contributor

sbfnk commented Jan 26, 2024

But they all return stats::rnorm? I think that's where you need substitute(), in which case we end up with my suggestion above.

@pratikunterwegs
Copy link
Collaborator

My mistake. I guess the options would then be to pass a string, or your solution.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 26, 2024

Thanks for having a look and trying to come up with alternatives!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high-priority
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants