nimble icon indicating copy to clipboard operation
nimble copied to clipboard

Too much potential for model node name conflicts from lifted nodes

Open danielturek opened this issue 7 years ago • 6 comments

The title says it all. Just like how we rename the internal (lifted, and otherwise) node functions with suffixes such as "xxx_ L11", and also for lifted nodes with prefixes such as "lifted_xxx", I think we need to be more careful to give unique names to new nodes created from LHS transformations.

For example, I don't think the lifted nodes named "log_a" and "logit_b" in this example are unique enough. I would suggest, perhaps, the suffix with line numbers, like "log_a_L2", or something like that, if not using the "unique ID" system.

Example:

library(nimble)

code <- nimbleCode({
    log(a) ~ dnorm(0, 1)
    logit(b) ~ dnorm(0, 1)
})

Rmodel <- nimbleModel(code)

Rmodel$getNodeNames()

## [1] "log_a"   "logit_b" "a"       "b"     

danielturek avatar Nov 03 '18 14:11 danielturek

Additional historical perspective: this current system for handling the LHS-transformations dates way back, to the "new model def" system, c 2014.

While clear and functional, all our other systems for auto-generation of names have since been updated and modernized to prevent accidental conflicts. But this system appears to have remained in its original form.

danielturek avatar Nov 03 '18 14:11 danielturek

For LHS nodes as in your examples, we could do that, but I'm not convinced we need to. Is there a case where a name clash can be demonstrated?

For RHS nodes, the lifted names are used to determine redundancies, so that the same lifting is not done multiple times. E.g.

for(i in 1:5) {
  x[i] <- dnorm(mu_x, tau)
}
for(i in 1:5) {
  y[i] <- dnorm(mu_y, tau)
}

The lifted sd node for 1/sqrt(tau) will be lifted only once, and the matching name transformations are what identify the second case as having the same need as the first case. The intention was that the lifted node name is a uniquely string-ified version of the expression.

perrydv avatar Nov 21 '18 14:11 perrydv

Example of name clash:


library(nimble)

code <- nimbleCode({
    log(a) ~ dnorm(0, 1)
    logit(b) ~ dnorm(0, 1)
    logit_b ~ dexp(3)
})

Rmodel <- nimbleModel(code)

## Error: There are multiple definitions for nodes:logit_b
## If your model has macros or if-then-else blocks
## you can inspect the processed model code by doing
## nimbleOptions(stop_after_processing_model_code = TRUE)
## before calling nimbleModel.

danielturek avatar Nov 21 '18 14:11 danielturek

Thanks. That's helpful. How about the lifted node can end in "lifted", e.g. "logit_b_lifted_"?

perrydv avatar Nov 24 '18 22:11 perrydv

Sure, sounds good. A unique identifier postfix "UID##" would be additionally safe.

danielturek avatar Nov 25 '18 00:11 danielturek

I think the "UID##" suggestion would make it difficult to use the name as a one-to-one encoding of the expression for purposes of determining when a lifted node for some expression already exists.

perrydv avatar Nov 27 '18 19:11 perrydv