Closures (again) #742
Closures (again) #742
Conversation
|
Is this + the math PR everything we need to now review this? Any open issues or questions? Maybe me and @bbbales2 can co-review this? This may be too much frontend for my paygrade but maybe I could manage. |
|
@rok-cesnovar yeah if you want to look at it please. My goal in the next few days is to get it to build and then try writing a few models and see how it works. If it works good then I'll start pressuring people who know Ocaml to do the review. If that's you great lol. |
|
Glad to help. I feel bad this is sitting here for so long. You playing with models and me starring at the code sounds like a plan. If I fail we can always bug Ryan :) |
|
Binaries for this PR are available here: https://github.com/stan-dev/stanc3/actions/runs/385882467 |
|
Built the latest binaries here: https://github.com/stan-dev/stanc3/actions/runs/396298848 Closures in transformed parameters works now so the sir model runs which is cool. Is the This is a model I was using to try out data {
int N;
int n_redcards[N];
int n_games[N];
vector[N] rating;
}
parameters {
vector[2] beta;
}
model {
int grainsize = 1;
functions
real partial_sum(int[] slice_n_redcards,
int start, int end) {
return binomial_logit_lpmf(slice_n_redcards |
n_games[start:end],
beta[1] + beta[2] * rating[start:end]);
}
beta[1] ~ normal(0, 10);
beta[2] ~ normal(0, 1);
target += reduce_sum(partial_sum, n_redcards, grainsize);
}Errors look like: clang++ -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 stan/lib/stan_math/ -I stan/lib/stan_math/lib/eigen_3.3.7 -I stan/lib/stan_math/lib/boost_1.72.0 -I stan/lib/stan_math/lib/sundials_5.2.0/include -DBOOST_DISABLE_ASSERTS -c -include-pch stan/src/stan/model/model_header.hpp.gch -x c++ -o redcard_lambda.o redcard_lambda.hpp
redcard_lambda.hpp:98:19: error: no matching constructor for initialization of 'std::vector<double>'
: beta(beta__), n_games(n_games__), rating(rating__),
^ ~~~~~~~~~
Data is |
|
Can you add some information to this about the syntax and design and all that? Does this follow https://github.com/stan-dev/design-docs/blob/master/designs/0004-closures-fun-types.md ? Also - let me know if you could use help or review on this. Would love to get closures in. |
|
@seantalts more conversation over here: stan-dev/math#2197 , @bob-carpenter said he'd do a review for it, so send him a message if you want to do language review. @nhuurre laid out the differences in the design doc and the implementation here: stan-dev/math#2197 (comment) . The doc should get updated to reflect what ends up happening. |
|
Sweet, looks good to me. Happy to let Bob or whoever do the review, was just offering a helping hand on the language part and didn't realize that discussion was taking place there. But yeah, in general feel free to tag me if there are lingering PRs or what-have-you. |
Yikes, I meant to get to this sooner. @nhuurre --- let me know how I can help. I'm happy to review PRs or testing plans or whatever. |
|
Just to check, this only does closures and not the lambda syntax from the design doc. I'm totally fine with that just wanted to check |
| function | ||
| real bar(real z, row_vector r) { | ||
| real rs = sum(r); | ||
| real ys = sum(y); | ||
| real vs = sum(v); | ||
| real pas = sum(pa); | ||
| real pvs = sum(pv); | ||
| return z + rs + x + ys + vs + p + pas + pvs; | ||
| } |
There was a problem hiding this comment.
Do you have an example of a failure when there is shadowing in the function and a value is brought in from a higher scope? Like
| function | |
| real bar(real z, row_vector r) { | |
| real rs = sum(r); | |
| real ys = sum(y); | |
| real vs = sum(v); | |
| real pas = sum(pa); | |
| real pvs = sum(pv); | |
| return z + rs + x + ys + vs + p + pas + pvs; | |
| } | |
| function | |
| real bar(real z, row_vector r) { | |
| vector[N] pa = // ... fill this with something | |
| real rs = sum(r); | |
| real ys = sum(y); | |
| real vs = sum(v); | |
| real pas = sum(pa); | |
| real pvs = sum(pv); | |
| return z + rs + x + ys + vs + p + pas + pvs; | |
| } |
There was a problem hiding this comment.
Also can you give the test function names that line up with what they intend to check or validate? It makes it easier when looking across the stan/mir/cpp
There was a problem hiding this comment.
Also are there languages besides R that allow the pulling in from a higher scope thing? imo looks weird but if people want it then I won't object
There was a problem hiding this comment.
C++ also allows this in lambdas for constexpr defined objects, but that's a very specific type of scoping that's known globally at compile time
There was a problem hiding this comment.
The Stan language does not allow shadowing. Closures are no exception.
There was a problem hiding this comment.
Would you mind adding an example that takes in no arguments and uses just scoped objects? Reading over the gen'd C++ right now and would be easier to parse through that logic.
There was a problem hiding this comment.
It won't let me comment on the C++ file because it's too many changes, for review (or maybe just a good idea) it would be good to print those into a separate closure.expected file
There was a problem hiding this comment.
I'll comment on one here though
template <typename T0__, typename T1__, typename T2__, typename T3__,
typename T6__, typename T7__>
stan::promote_args_t<T0__, T1__, stan::value_type_t<T2__>,
stan::value_type_t<T3__>,
T6__, stan::promote_args_t<stan::value_type_t<T7__>>>
bar_L23C9_impl__(const T0__& p, const std::vector<T1__>& pa, const T2__& pv,
const T3__& v, const double& x,
const std::vector<double>& y, const T6__& z, const T7__& r,
std::ostream* pstream__) ;
template<bool ref__, typename F0__, typename F1__, typename F2__>
class bar_L23C9_cfunctor__ {
/**
* When would these be captured by value?
*/
stan::capture_type_t<F0__, ref__> p;
stan::capture_type_t<std::vector<F1__>, ref__> pa;
stan::capture_type_t<Eigen::Matrix<F2__, -1, 1>, ref__> pv;
const Eigen::Matrix<double, -1, 1>& v;
stan::capture_type_t<double, ref__> x;
const std::vector<double>& y;
public:
const size_t vars_count__;
bar_L23C9_cfunctor__(const bar_L23C9_cfunctor__<ref__, F0__, F1__, F2__>&) = default ;
bar_L23C9_cfunctor__(bar_L23C9_cfunctor__<ref__, F0__, F1__, F2__>&&) = default ;
bar_L23C9_cfunctor__(const F0__& p__, const std::vector<F1__>& pa__,
const Eigen::Matrix<F2__, -1, 1>& pv__,
const Eigen::Matrix<double, -1, 1>& v__,
const double& x__, const std::vector<double>& y__)
: p(p__), pa(pa__), pv(pv__), v(v__), x(x__), y(y__),
vars_count__(count_vars(p__, pa__, pv__, v__, x__, y__)) {}
template <typename T0__, typename T1__>
stan::promote_args_t<F0__, F1__, F2__, T0__,
stan::value_type_t<T1__>>
operator()(std::ostream* pstream__, const T0__& z, const T1__& r) const
{
return bar_L23C9_impl__(p, pa, pv, v, x, y, z, r, pstream__);
}
/**
* Will any of these be used in places we will also have user defined code?
* if not, then no need to double underscore.
*/
using captured_scalar_t__ = stan::return_type_t<F0__, F1__, F2__>;
using ValueOf__ = bar_L23C9_cfunctor__<false, double, double, double>;
using CopyOf__ = bar_L23C9_cfunctor__<false,
stan::capture_type_t<F0__, false>,
stan::capture_type_t<F1__, false>,
stan::capture_type_t<F2__, false>>;
/**
* Why not just have a static constexpr bool stan_closure{true};
* in here so stan math can detect if this is a stan closure?
*/
/**
* idt we need these? I think you could just have a function that
* returns a tuple of the members like
*/
auto get_members() {
return std::make_tuple(p, pa, pv, v, x, y);
}
/**
* Then on the stan math side we can just run stuff like
*/
// in math deep_copy_vars.hpp file
template <typename F, require_stan_closure_t<F>* = nullptr>
auto deep_copy_vars(F&& f) {
return stan::math::for_each([](auto&& x) { return deep_copy_vars(x);}, f.get_members());
}
size_t count_vars__() const {
return vars_count__;
}
auto value_of__() const {
return ValueOf__(eval(value_of(p)), eval(value_of(pa)), eval(value_of(pv)),
v, x, y);
}
auto deep_copy_vars__() const {
return CopyOf__(eval(deep_copy_vars(p)), eval(deep_copy_vars(pa)),
eval(deep_copy_vars(pv)), v, x, y);
}
void zero_adjoints__() {
stan::math::zero_adjoints(p);
stan::math::zero_adjoints(pa);
stan::math::zero_adjoints(pv);
stan::math::zero_adjoints(v);
stan::math::zero_adjoints(x);
stan::math::zero_adjoints(y);
}
double* accumulate_adjoints__(double *dest) const {
return stan::math::accumulate_adjoints(dest, p, pa, pv, v, x, y);
}
stan::math::vari** save_varis__(stan::math::vari **dest) const {
return stan::math::save_varis(dest, p, pa, pv, v, x, y);
}
};
Yes, no lambda syntax. I mentioned the differences from the design doc here. (but that's outdated, the special suffixes |
I renamed the files and functions to something a bit more explanatory and added a unit test. The tests are admittedly rather haphazard. |
| const std::vector<double>& y; | ||
| public: | ||
| const size_t vars_count__; | ||
| capture_data_L9C9_cfunctor__(const capture_data_L9C9_cfunctor__&) = default ; |
There was a problem hiding this comment.
Does the compiler not handle these automatically?
There was a problem hiding this comment.
Probably? I think I was just playing it safe here. Not sure if the constructor is even needed anywhere...
There was a problem hiding this comment.
word yeah idt these are necessary. If you don't define any of the default generated constructors then it should just make them for you
|
I just tried this out and I really like it. While I got it working I was slightly irritated by two things:
Sorry if this was already discussed and I missed it, but what is the rationale here? The above two points are just what I would have expected naively. Other than that, it is really cool to use! |
Function definitions in the
I think a local block would have confusing scope--local variables are not visible outside of the (local) block in which they are declared but the functions from the top-level |
This is the way C/C++ does it, too. I think it's more natural that way. If you required a semicolon, it'd be like those pesky class-final semicolons in C++. |
Everything declared before the block should be in scope. Having a local block would make it consistent with the profiling thing we have, no? Is that a consideration maybe? I really think we want closures sooner than later, but I also still think a design doc would be great (or have I missed that)? If I missed it, then shame on me! The intent of the design doc is just to make enough people aware of this and see this. I know it's some work, but it is all in scope for 2.28 for sure. |
|
With profiling or any local block we have this: model {
profile("test") {
real a = 5; // this is a local sc
}
real b = a + 2; // this would error
}Similarly with local functions model {
functions {
real foo() {
return 0.0;
}
}
real b = foo(); // foo should be out of scope.
}Allowing
A design doc for this was done in 2019 by @bob-carpenter: stan-dev/design-docs#6 Once this PR is merged I think the important details should probably be added to the design doc so it reflects what was added. |
|
Updating the design doc would be great... and thanks for pointing that doc out; I was not sure if that was it. I haven't put my head too deep in to that doc, so if my comments are late, then sorry for that (EDIT: I did go over the doc before continuing to write this). I see your point about that it's odd to declare a block and then use it later on. One possibility I see is to limit the flexibility somewhat, but make it more consistent with the current structure. The way to do that is by simply allowing additional functions block between any other block. So this would be legal: // global functions without any scope of other variables
functions {
}
data {
}
// can access data
functions {
}
transformed data {
}
// can access all of the above
functions {
}
//.... and so onThis limits the closures to be defined in blocks and it seems as if this would also be consistent with the major deviation here which is no lambdas can be re-assigned. Going this way would also solve scoping issues which can happen whenever a function is defined within a local blocks of one of the main block // ...
model {
real a =1;
{ // local block
// define function foo
}
// can I use foo here?
}@rok-cesnovar the example you post above could be written like model {
{ // local block
functions {
real foo() {
return 0.0;
}
}
real b = foo(); // foo should be out of scope.
}
}So in addition to allowing function blocks between any other blocks, we could also allow them at the start of a local block as above. This would eliminate the restriction that a function could not be defined within a main block. Thoughts? (I hope this is helpful... if we deemed all of this set in stone then apologies) EDIT: And one more thing: The limitiation of this implementation for closures to not introduce a type for functions is fine for me. We are making great progress here with the closures. However, it would be great if some thought can be put into how that bit can come to live at a later stage possibly. The document from Bob would make Stan far more functional programming like and this implementation does not do that. That's fine per se, but in case we limit ourselves for the future, then that needs to be spelled out. If not, even better as we can do it later. |
|
I'd like to echo the things @wds15 said - I think a PR should be opened to design-docs to update the proposal for discussion. As part of that it would be great to talk about how compatible this implementation would be with function types or true anonymous lambdas |
The stanc3 part of
stan-dev/math#2094stan-dev/math#2384Copyright 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)