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

problematic lookup/scoping for nested nimbleFunctions #1497

Open
paciorek opened this issue Oct 20, 2024 · 2 comments
Open

problematic lookup/scoping for nested nimbleFunctions #1497

paciorek opened this issue Oct 20, 2024 · 2 comments

Comments

@paciorek
Copy link
Contributor

If a user defines a nimbleFunction whose name is the same as the name of a function in the nimble DSL, in compiled code we use the DSL function and not the user's nimbleFunction. E.g., exp, log1p, gamma, etc.

This is a problem for (at least) two reasons:

  • A user presumably expects their defined nimbleFunction to take precedence and may not even know the function exists in the DSL. No warning is given.
  • Uncompiled execution, via R scoping, will use the user's nf, so uncompiled behavior will differ from compiled.
log1p <- nimbleFunction(
    run=function(x=double(0)) {
        returnType(double(0))
        return(9999)
    })

nf <- nimbleFunction(
    run=function(x=double(0)) {
        out = log1p(x)
        print(out)
    })

cnf <- compileNimble(nf)
cnf(1)  # user would expect 9999
# 0.693147

Some options:

  • Simply look to see if an RC function of the same name exists and warn the user to name it differently.
    • This is the minimally intrusive change...
  • Reorder the processing in exprClasses_setSizes to look for RC functions first.
    • This feels more dangerous, but I think it is the conceptually correct thing to do.
    • If we do this, we'll need to address the fact that in line 280 of genCpp_sizeProcessing.R, the lookup (via R's scoping) when using get(code$name) will find names in base:: before names in GlobalEnv, which will cause problems with defaulting to using user-defined RC functions.

gamma() is a more complicated variation on this given we translate gamma -> gammafn earlier on...

@perrydv this is not urgent, but we'll want to discuss for 1.3.0.

@paciorek
Copy link
Contributor Author

Followup: given nCompiler, I think I will propose:

  1. We use the minimal approach and warn users.
  2. We keep this in mind for nCompiler lookup and handle things more robustly there.

@paciorek
Copy link
Contributor Author

See related issue #1469, including the possibility of using getAnywhere rather than get.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant