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

Confusing/new behavior/error with registered distribution #1355

Closed perrydv closed 9 months ago

perrydv commented 10 months ago

Here is a minimal version of an issue raised by a user. When attempting to clear the environment, the presence of a previously registered distribution internally causes an inscrutable error during the second round of compilation. The user reports that their code worked previously, so this problem may have been introduced in a recent version of nimble.

Cursory investigation did not reveal an obvious change that would have introduced this. There were (less obvious) changes for AD to check and utilize user-defined distributions, so that might be the source of the problem.

library(nimble)

dfoo <- nimbleFunction(
  run = function(x=double(), log=integer(0, default=0)) {
    return(0)
    returnType(double())
  }
)

m <- nimbleModel(
  nimbleCode({
    x ~ dfoo()
  })
)

cm <- compileNimble(m)

rm(list = ls())
# Then rerun from dfoo through through compileNimble to see the error

# The error does not happen if:
# rm(list=ls()) is not run, OR
# deregisterDistributions("dfoo") is run
paciorek commented 10 months ago

The error occurs in 0.13.2, so probably not related to AD.

Here's the error:

Error: In sizeAssignAfterRecursing: 'rfoo' is not available or its output type is unknown.
 This occurred for: model_x[1] <<- rfoo(1)
 This was part of the call:  {
  model_x[1] <<- rfoo(1)

This is because we've created a dummy rfoo, which is registered, and then rm removes it "uncleanly".

paciorek commented 10 months ago

I'm not inclined to "fix" this as I would argue this is not a bug. We told the user initially we are creating rfoo for them and that if they change the distribution they need to call deregisterDistributions. They then chose to blow away the environment's objects. And the error reasonably informative. It says that rfoo is not available, albeit the error is not trapped where we probably would want it to be.

We could trap this in checkUserDefinedDistribution with some specific logic, but the code that would be involved feels clunky as it would duplicate some of what registerDistributions does.

paciorek commented 9 months ago

Check in sizeASsignRecursing if the issue seems to relate to registered distributions.

paciorek commented 9 months ago

I'm modifying the error trapping in sizeAssignAfterRecursing to give a more refined message in this case.