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

Check target efficiency #47

Merged
merged 5 commits into from
Apr 25, 2024
Merged

Check target efficiency #47

merged 5 commits into from
Apr 25, 2024

Conversation

danielturek
Copy link
Member

Improves efficiency of hmc_checkTarget.

@perrydv
Copy link
Contributor

perrydv commented Feb 12, 2024

Thanks for this @danielturek. It generally looks like it will be much more efficient. Here are some suggestions / requests:

  • Can we add an argument to entry point functions (configureHMC and others) to turn off checking if it's not wanted, similar to the check argument to nimbleModel? It is really bad if the checking itself has a bug or wastes time when a user is confident about their use case, so it is good to be able to skip it.
  • I can see bits of room for more efficiency (e.g avoiding sapply, and doing the c('dweib', 'dmnorm', 'dmvt', 'dwish', 'dinvwish') once instead of every time through the loop), but these are likely minor in comparison to your changes to be checking at the level of declarations.
  • I am not sure if the get(dists[i], envir = parent.frame(4)) is completely universal. Would that be right if configureHMC was called from one or more layers into a user's own functions, not necessarily from the same environment as nimbleModel was called to define their model? I am not 100% sure but a better way to find user-defined dists might be to look in the userEnv from the call to nimbleModel. I think that would provide the intended scoping, i.e. a search for user-defined dists should start from where nimbleModel was called. Although the model or modelDef do not retain the userEnv, it is retained in the envir field of each of the BUGSdeclClass elements in the model$modelDef$declInfo list.
  • I think get throws an error if it can't find what was requested. So we have often used exists first and then if that is TRUE we do the get next. I think mget is another solution, with an ifnotfound argument.
  • From nimble:::is.rcf, it looks like we check for nfMethodRCobject using exists('nfMethodRCobject', envir = environment(nfObj), inherits = FALSE). I'm not sure if that's better or equivalent to your check.
  • When the change comes to nimble to allow user-defined distributions that include setup code, your checks based on nfMethodRCobject will not work because those are specific to RCfunctions (nimbleFunctions without setup code). We could anticipate that change to nimble in a couple of ways. We could provide an option that turns off the specific checking tied to RCfunctions in nimbleHMC. Or we could right now build in a check for the more general case (and I guess also provide an option to turn it on or off). Another option (perhaps simpler, but less satisfying?) would be to simply omit this part of the checking in nimbleHMC for now. Note that the generated C++ will error out if derivatives are taken with respect to a distribution parameter or value where they are not supported. That is a rougher way to trap the error, but it is something.

@danielturek danielturek merged commit 41c782e into master Apr 25, 2024
4 checks passed
@danielturek danielturek deleted the checkTarget_efficiency branch April 25, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants