The --info option produces a list of used functions and distributions

[mandel]

Release notes

As part of the issue #808, it was discuss to add the list of "all named functions used". I understood that as the list for functions called from the standard library and the list of used distribution.

This PR adds to the JSON object produced by the --info option the two following fields:

• stanlib_calls: list of functions of the standard library called in the program
• distributions: list of distributions to the right of a ~ statement.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

[mandel]

I do not understand why the changes in this PR cause the following error:

--- Translating Stan model to C++ code ---

bin/stanc  --o=examples/bernoulli/bernoulli.hpp examples/bernoulli/bernoulli.stan


--- Compiling, linking C++ code ---

clang++-6.0 -march=core2 -std=c++1y -D_REENTRANT -Wno-sign-compare -Wno-ignored-attributes      -I stan/lib/stan_math/lib/tbb_2019_U8/include   -O3 -I src -I stan/src -I lib/rapidjson_1.1.0/ -I lib/CLI11-1.9.1/ -I stan/lib/stan_math/ -I stan/lib/stan_math/lib/eigen_3.3.9 -I stan/lib/stan_math/lib/boost_1.75.0 -I stan/lib/stan_math/lib/sundials_5.6.1/include    -DBOOST_DISABLE_ASSERTS         -c -include-pch stan/src/stan/model/model_header.hpp.gch -x c++ -o examples/bernoulli/bernoulli.o examples/bernoulli/bernoulli.hpp

fatal error: file '/usr/include/linux/futex.h' has been modified since the precompiled header 'stan/src/stan/model/model_header.hpp.gch' was built

It looks unrelated to the --info option.

[rok-cesnovar]

This is unrelated, maybe a one-off error on our test machine or a configuration issue on the machine. In both cases, dont worry about that, we will look into that. EDIT: Passing now, so it was a one-off thing, sorry.

Thanks for the PR! Looks nice.

Two comments/questions:

• I think we dont need to separate distributions? Or do you think there is a use case? Might be easier to just list dirichlet_lpdf in stanlib_calls? You can call this for all tilde statements:
  https://github.com/stan-dev/stanc3/blob/master/src/middle/Utils.ml#L48

• We probably need to add a test for higher-order functions. Example models to use: https://github.com/stan-dev/stanc3/blob/master/test/integration/good/code-gen/reduce_sum_m1.stan or https://github.com/stan-dev/stanc3/blob/master/test/integration/good/ode/ode_rk45_test_model.stan

[mandel]

I think we dont need to separate distributions? Or do you think there is a use case?

I do not have a strong opinion but it is easier to add a post-processing phase that merge them than the converse. Moreover, in our Stan to (Num)Pyro compiler with keep the two forms separated and might have a benefit to have access to this information.

Might be easier to just list dirichlet_lpdf in stanlib_calls? You can call this for all tilde statements: https://github.com/stan-dev/stanc3/blob/master/src/middle/Utils.ml#L48

It seems that this function does not add the _lpdf suffix when called on "dirichlet".

[mandel]

We probably need to add a test for higher-order functions. Example models to use: https://github.com/stan-dev/stanc3/blob/master/test/integration/good/code-gen/reduce_sum_m1.stan or https://github.com/stan-dev/stanc3/blob/master/test/integration/good/ode/ode_rk45_test_model.stan

That is an interesting question. In these examples, the functions in argument of reduce_sum and ode_rk45 are local functions so not reported anyway. But if the function in argument is a function from the Stan standard library it would currently not be present in the list stanlib_calls. Would you prefer a different behavior?

[seantalts]

I do not have a strong opinion but it is easier to add a post-processing phase that merge them than the converse.

+1, I think it's neat to present additional information that could be helpful. I'm not sure if this is better or worse but we could also structure it something like
functions_called : [{"dirichlet": {"distribution": true}, "log": {"distribution": false}]

[rok-cesnovar]

I do not have a strong opinion but it is easier to add a post-processing phase that merge them than the converse.

Very good point. Ignore my previous comment. Splitting these up is good.

the functions in argument of reduce_sum and ode_rk45 are local functions so not reported anyway.

Ah yes, I was overthinking this. Obviously these call user-defined functions so the current behaviour is fine. We were discussing functors for Stan lib functions in stan-dev/design-docs#35 but we can address that if we actually do start supporting it.

[rok-cesnovar]

Thanks again for working on this!

Sorry I suggested the wrong function in my comment. This is the example (we only care of the StanLib ones):

let possible_names =
    List.map ~f:(( ^ ) distribution.name) Utils.distribution_suffices
    |> String.Set.of_list
in
if List.exists ~f:(fun (n, _) -> Set.mem possible_names n) ud_dists (* ud_dists is the 'dirichlet' in our case *)
then Fun_kind.UserDefined
else StanLib

I am nitpicking here because I think

    l ~ dirichlet(c);
    target += normal_lpdf(y| 0, 1);

should both return functions either with or without the suffix. I prefer with suffix but would be fine without it as well as long as its consistent.

Also please add a test for Stan lib functions in user-defined functions:

functions {
  real foo(real a) {
    return sin(a);
  }
  real goo_lpdf(real a) {
    return a;
  }
}

If I then add

    y ~ goo();

goo should not be listed.

[mandel]

Thanks @rok-cesnovar. I have implemented most of your suggestions. But for now we still distinguish distributions with and without post-fix.

If you do not like the current solution, I also prefer the solution where the post-fix is added since it is always easy to remove the post-fix later.

But in this case, I think that it would be good to expose the function dist_name_suffix (https://github.com/stan-dev/stanc3/blob/master/src/frontend/Ast_to_Mir.ml#L501-L511) such that this code is not duplicated. I don't know if it should be added to the interface of Ast_to_Mir or moved to Middle.Utils?

[rok-cesnovar]

Thanks!

I think we want to have the _lpdf/_lpmf suffixes. My reasoning is that these two models are equivalent, produce the exact same C++ code and should also produce the same output with --info.

parameters {
    real y;
}
model {
    y ~ std_normal();
}

parameters {
    real y;
}
model {
    target += std_normal_lupdf(y);
}

But in this case, I think that it would be good to expose the function dist_name_suffix (https://github.com/stan-dev/stanc3/blob/master/src/frontend/Ast_to_Mir.ml#L501-L511) such that this code is not duplicated. I don't know if it should be added to the interface of Ast_to_Mir or moved to Middle.Utils?

I would say Middle.Utils but not sure. Do you have a preference @seantalts ? Utils has all the other distribution suffix utilities, idk.

[rok-cesnovar]

My above comment got me thinking about another test case. Sorry I missed this earlier.

[mandel]

In fact, it is not possible to move dist_name_suffix in Middle.Utils because it introduces a cyclic dependency with Middle.Stan_math_signatures. So should it be in Middle.Stan_math_signatures?

[rok-cesnovar]

So should it be in Middle.Stan_math_signatures?

I think so yes.

I would also rename stanlib_calls to functions. I think then its good to go.

[rok-cesnovar]

Thank you Louis!

[mandel]

You're welcome!