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

Issue with shadowing and binary functions #997

Closed
rok-cesnovar opened this issue Oct 11, 2021 · 5 comments · Fixed by #1011
Closed

Issue with shadowing and binary functions #997

rok-cesnovar opened this issue Oct 11, 2021 · 5 comments · Fixed by #1011
Labels
bug Something isn't working cpp-codegen robustness

Comments

@rok-cesnovar
Copy link
Member

rok-cesnovar commented Oct 11, 2021

This is a continuation of the discussion with @WardBrian here: #995 (comment)

Long story short, this model's generated code fails to compile:

transformed data {
   real multiply;
   real a = multiply(0, 1);   
}

with

examples/test/test.hpp: In constructor ‘test_model_namespace::test_model::test_model(stan::io::var_context&, unsigned int, std::ostream*)’:
examples/test/test.hpp:66:24: error: expression cannot be used as a function
   66 |       a = multiply(0, 1);
      |                        ^

We allow shadowing and it works with nullary and unary functions. For example this model works fine:

transformed data {
   real log;
   real a = log(10);
   real e;
   real f = e();
   real positive_infinity;
   real g = positive_infinity();  
}

Solutions to this are:
a) add stan::math namespace to a function if there is a variable with the same name used in the model
b) add stan::math namespace to all Stan Math function calls (this probably doesnt work as we may fallback to std:: in some cases)
c) if a function is shadowed it can no longer be used as a function in that model (@WardBrian mentioned this is what Python does)
d) disallow shadowing for binary functions (we have to check functions with 3+ args as well)

This is not a new issue for this release, its been present since we allowed shadowing.

@rok-cesnovar
Copy link
Member Author

For the record, I would go with a) if its easy to implement, if not then c).

@WardBrian WardBrian added bug Something isn't working robustness labels Oct 11, 2021
@SteveBronder
Copy link
Contributor

I think I would do (b) here, all of the functions available through stan and std are available in stan::math

@rok-cesnovar
Copy link
Member Author

rok-cesnovar commented Oct 12, 2021

I am not so sure we are supporting all signatures in Math at the moment. At least in the past we had a few things like pow(int, int). But maybe we cleaned that up.

But yeah, there are not that many std:: function and we might as well just check them all.

@WardBrian WardBrian mentioned this issue Oct 12, 2021
2 tasks
@WardBrian
Copy link
Member

@SteveBronder did you ever get a chance to look into this?

@WardBrian
Copy link
Member

We currently have this chunk of code:

let functions_requiring_namespace =
String.Set.of_list
[ "e"; "pi"; "log2"; "log10"; "sqrt2"; "not_a_number"; "positive_infinity"
; "negative_infinity"; "machine_precision"; "abs"; "acos"; "acosh"; "asin"
; "asinh"; "atan"; "atanh"; "cbrt"; "ceil"; "cos"; "cosh"; "erf"; "erfc"
; "exp"; "exp2"; "expm1"; "fabs"; "floor"; "lgamma"; "log"; "log1p"; "log2"
; "log10"; "round"; "sin"; "sinh"; "sqrt"; "tan"; "tanh"; "tgamma"; "trunc"
; "fdim"; "fmax"; "fmin"; "hypot"; "fma"; "complex" ]
let stan_namespace_qualify f =
if Set.mem functions_requiring_namespace f then "stan::math::" ^ f else f

Which I believe is why the unary functions still do compile (though it seems outdated/wrong, complex isn't a function). It seems like instead of a whitelist we could make this just call on all arguments, and if we need some std:: instead we could make a list of those and branch on that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cpp-codegen robustness
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants