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

allow offspring argument to be a function #25

Closed
pearsonca opened this issue Mar 14, 2020 · 10 comments · Fixed by #188
Closed

allow offspring argument to be a function #25

pearsonca opened this issue Mar 14, 2020 · 10 comments · Fixed by #188
Labels
enhancement New feature or request pkg_infrastructure
Milestone

Comments

@pearsonca
Copy link

the offspring argument to chain_sim should support passing a function of n (like serial argument).

@jamesmbaazam jamesmbaazam added the enhancement New feature or request label Jan 4, 2023
@sbfnk
Copy link
Contributor

sbfnk commented Jan 12, 2023

It kind of does already, if you define a function starting with r and pass the corresponding string. A question is whether the interface could be simplified by using functions instead of strings here, but that would mess with the analytical likelihoods e.g. in https://github.com/sbfnk/bpmodels/blob/466896dd57c5dd51ef9cbe7c0cff4cdd41bbfda4/R/likelihoods.R#L8

@pearsonca
Copy link
Author

so people would need to provide both the sampling and likelihood functions (potentially - probably someway to introspect some common ones) - that seems plausibly doable, if there's demand?

@jamesmbaazam jamesmbaazam transferred this issue from epiforecasts/bpmodels May 2, 2023
@sbfnk
Copy link
Contributor

sbfnk commented Jun 6, 2023

so people would need to provide both the sampling and likelihood functions (potentially - probably someway to introspect some common ones) - that seems plausibly doable, if there's demand?

Instead of requiring that, and in the spirit of what I understand to be the underlying issue (offspring and serial calling for different approaches), how about changing serial to take a character string (with corresponding random and distribution functions expected to be present in the environment)? My sense is that this would be simpler.

@pearsonca
Copy link
Author

sure, but: why not both? R is pretty good at introspection.

could approach as

  • have some internal infrastructure to check for a string
  • if a string, promote to the function version; otherwise use argument as the function
  • then have the rest of internal code work always work with the function version

@sbfnk
Copy link
Contributor

sbfnk commented Jun 6, 2023

Yes, could do, though it would require some overhead as some function access the r version, some the d version, some both - so we'd need to pass either one or either of them.

We could define a distribution S3 object which has optional sample, density etc. containers. Would you say the benefit from doing so is worth the additional overhead (or have a better idea)?

@pearsonca
Copy link
Author

weeeeeell, is there a "most typical" use case for particular arguments? e.g. seems like offspring and serial ought to typically be the r flavor, and likelihood the d flavor?

if users want to use some atypical approach, they can always just pass a whole function.

@sbfnk
Copy link
Contributor

sbfnk commented Jun 6, 2023

The likelihood function needs both as it relies on simulations to estimate the likelihood e.g. in the presence of underreporting.

I'm not at all convinced the character string approach is ideal as it relies on functions being present in the environment which I'm sure can create complications in edge cases, and can requre e.g. loading whole packages into the environment even where only one or two functions are needed - but I'm also struggling to be convinced that the additional overhead from having to deal with multiple argument types is worth it - it would e.g. complicate integration with the truncdist package suggeted in #3 (comment) which also uses the character string approach.

Just to be clear, the current set up for offspring (passing a character string) doesn't stop you from defining your own d and r functions and passing their common name.

@pearsonca
Copy link
Author

my main complaint is inconsistent argument types are required, less so that I want a particular flavor supported (though my flavor preference is functions - sweet, sweet lambda action).

@sbfnk
Copy link
Contributor

sbfnk commented Jul 26, 2023

See also discussion at #33 (comment) which I'll continue here

@sbfnk
Copy link
Contributor

sbfnk commented Jul 26, 2023

An additional issue with a lambda approach is that it precludes the lookup for analytical likelihoods included with the package.

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

Successfully merging a pull request may close this issue.

3 participants