search

nimble-dev / nimble

The base NIMBLE package for R
http://R-nimble.org
BSD 3-Clause "New" or "Revised" License
156 stars 23 forks source link

save userEnv in modelDef object #1414

Closed danielturek closed 4 months ago

danielturek commented 7 months ago

The title says it all.

The motivation for use in hmc_checkTarget, to find user-defined distributions in a more robust manner.

perrydv commented 7 months ago

@danielturek In one way this might be moot because we realized that nimbleHMC can find the userEnv for a model from the BUGSdeclClass objects, so this could be done from nimbleHMC. But more than what this PR title says, providing the check as a modelDef method would make sense. But now that I have PR #1348 passing, I could update the code in this PR to handle the case of user-defined dists with setup code.

danielturek commented 7 months ago

@perrydv Yes, I expanded the scope of this PR (after creating the title) to also add the checkADsupportForDistribution method for models.

I do think this would be a useful addition. If you're willing to update this PR to also handle user-defined distributions with setup code (nf$dist) that would be great. I eventually do hope to use this in nimbleHMC, which will have updates pending this addition.

paciorek commented 5 months ago

@perrydv flagging this for you to look at @danielturek 's suggestion to update for user-defined distributions.

perrydv commented 5 months ago

@paciorek I think this should be straightforward but there's just a merging workflow jam. The relevant feature for updating this branch is waiting in PR #1348. Let me know what you want to do but I think it would be fine to get both of these merged into devel and then I can work from a new branch at that point.

perrydv commented 4 months ago

@danielturek @paciorek It looks like the new checkADsupportForDistribution assumes that all built-in distributions have AD support (not true) and also that T() and I() have AD support (not true). Am I reading this correctly?

paciorek commented 4 months ago

I think you wrote that code (that's what git blame indicates). I'm not sure about the logic. Perhaps it is just meant as a partial check for user-defined dists, since we could, e.g., with nimbleHMC, only use derivs on part of a model so it's ok to have non-supported distributions (and T/I) in the other part of the model.

paciorek commented 4 months ago

It looks like @danielturek took out some code that set supported, which is no longer used, but is set to TRUE initially at the moment.

perrydv commented 4 months ago

Hah, that's pretty funny. Here is what I'm thinking. I don't want to make this more complicated, but I think with a small bit of effort we will end up with simpler code. I would like the registered distributions list to contain a new field about buildDerivs support for each distribution. Then there will be one place to look. Currently the modelDef$checkADsupportForDistribution has turned out a bit awkwardly in that it repeats the work done in registerDistributions to find relevant objects and it makes no use of modelDef internals so doesn't need to live as a method of that class. Because the work to find relevant objects is done in registerDistributions, this looks like the natural place to store that information for future easy access.

danielturek commented 4 months ago

@perrydv Your suggestion does seem to make sense, having this information instead be native to our (existing) distribution info. If I understand, then:

As @perrydv mentioned, this PR would then be largely moot; however, how do you guys feel about keeping the setUserEnv and getUserEnv, also added in this PR? It seemed like it could be helpful to have.

perrydv commented 4 months ago

Thanks for looking at this @danielturek . I realized that I was confused in making sense of the changes here. I thought more of the modelDef$checkADsupportForDistribution was new here than was the case, and in fact the changes there in this PR were just to conform with the verbosity setting and were only incidentally part of the PR. However, it made me realize the issue just discussed that we could store information about AD support in the dist objects. And also it made me realize (still checking this) that AD for nimbleFunction object methods in models lacked tests and would (or might) have encountered issues because modelDef$checkADsupportForDistribution was not updated for that case (which I thought was distinct on this PR's branch but wasn't). I also note that the userEnv feature that is the main addition of this branch, simple as it is, lacks a corresponding test.

My new plan is to merge this branch and then follow with a new PR with the other changes, and I will add a userEnv test along with other new tests in that PR.