Closures (again)

[nhuurre]

The stanc3 part of ~~stan-dev/math#2094~~ stan-dev/math#2384

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)

[rok-cesnovar]

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.

[bbbales2]

@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.

[rok-cesnovar]

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 :)

[rok-cesnovar]

Binaries for this PR are available here: https://github.com/stan-dev/stanc3/actions/runs/385882467

[bbbales2]

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 lupdf/lpdf behavior going to be based off the name still? Was messing with the reduce_sum example and was wondering.

This is a model I was using to try out reduce_sum. It looks like doing a closure over an array of integers breaks:

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 redcard.dat.txt

[seantalts]

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.

[bbbales2]

@seantalts more conversation over here: https://github.com/stan-dev/math/issues/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: https://github.com/stan-dev/math/issues/2197#issuecomment-745249124 . The doc should get updated to reflect what ends up happening.

[seantalts]

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.

[bob-carpenter]

Happy to let Bob or whoever do the review

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.

[bbbales2]

@bob-carpenter my attempt at a todo list here: https://github.com/stan-dev/example-models/pull/202, but @nhuurre can tell you more I'm sure

Currently the Math side of this supports ODEs and reduce_sum I think. integrate_1d, the algebra solvers, and map_rect support aren't there yet.

One thing I was curious about was how this implementation stacks up against the original specification. I did not check this. @nhuurre put comments here: https://github.com/stan-dev/math/issues/2197#issuecomment-745249124 .

Yikes

Ditto. I keep intending to help on the integrate_1d and algebra_solver front but keep getting unfocused on other stuff. Thanks for the patience @nhuurre

@nhuurre Draft instructions are here for Rok's test cmdstan tarball builder. It can produce a cmdstan tarball from a cmdstan/stanc3/stan/math branch that can be used from cmdstanr. (Edit: this makes it easy to share development stanc3 binaries without making people build the binaries themselves)

[nhuurre]

@rybern, you mentioned...uh, back in June, that you had some idea that could make the functions keyword unnecessary. If you have time could you take a look at this?

Some notes:

• a local variable cannot have function type; the declaration real(real, real) func = ...; is forbidden. Instead closure definitions must look just like functions, with the name before the argument list.
• the return type cannot be a function type. This was half-accidental and could be changed. However, if a closure can escape its initial scope then one has to worry about capture-by-reference leaving dangling pointers.
• there are no function arrays. The type array[] real(vector) refers unambiguously to a function/closure that takes a vector and returns an array of reals. This is a limitation in the C++ implementation.

[nhuurre]

Draft instructions are here for Rok's test cmdstan tarball builder. It can produce a cmdstan tarball from a cmdstan/stanc3/stan/math branch that can be used from cmdstanr.

I made a tarball here: https://github.com/nhuurre/cmdstan/releases/tag/closures-v0.1

[bbbales2]

I made a tarball here

This should be enough to install the tarball and use it in cmdstanr:

cmdstanr::install_cmdstan(release_url = "https://github.com/nhuurre/cmdstan/releases/download/closures-v0.1/cmdstan-stan-closures.tar.gz")

I tested it real quick and it worked.

And then use this to go back to the previous cmdstan, delete the ~/.cmdstanr/cmdstan-stan-closures folder (if you don't delete it cmdstanr might pick it up when it searches for a default cmdstan)

[rybern]

@nhuurre I spent a little while looking at the parser question - it's pretty gnarly! I'm 99% sure I found a workable solution, but it's especially hacky due to the earlier hack we did to allow "array" to be used as an identifier. Here are the pros/cons:

• Pro: write functions without the extra keyword
• Con: when the return type is an array, users would be able to write array [:,,:,:,] (mixing in colons instead of just commas) without a parse error, so writing array [:,:] would be the same as writing array[,]. I could fix this with yet more icky hacks, or we could ban array from being used as an identifier (@seantalts and I waffled over that in #714)
• Con: we'd get slightly weaker parser errors when a user swaps an unsized type for a sized type or vice versa, since the distinction can no longer be made by part of the parser
• Con: the parser would have yet more hacks in it

Details at https://github.com/rybern/stanc3/commit/bf4884b6195f6abe62213aebac4f2a0a92297c6f

[nhuurre]

Thanks @rybern

IMO the Ast.index type should have one more variant to distinguish between [] and [:]. (That's why you can reject array[1:] but not array[:], right?) I have complained about the complexity of the parser before. Maybe we should seriously consider adding a stage between parsing and AST.

Anyway, if it's too hacky then let's just keep the keyword. We can always deprecate it later if we don't need it anymore. Still, I'd like to at least mention the keyword in any error that results from omitting it. This is especially confusing:

Syntax error in 'err.stan', line 2, column 18 to column 19, parsing error:
   -------------------------------------------------
     1:  transformed data {
     2:    array[] real foo(array[] real x) {
                           ^
     3:      return x;
     4:    }
   -------------------------------------------------

Expected non-array type after array in return type.

[rybern]

IMO the Ast.index type should have one more variant to distinguish between [] and [:]. (That's why you can reject array[1:] but not array[:], right?)

Bingo, and I agree that's the workaround.

I have complained about the complexity of the parser before.

Yeah, doing this would make a couple parts of the parser harder to follow. See the branch I linked for roughly how it would look - the main trick is to pack all of the different variants of basic types (unsized, sized, and top-level) into one production, and then specialize them later. I'd want to add some big comments to explain how it works.

Still, I'd like to at least mention the keyword in any error that results from omitting it.

It's not clear to me how good of an error message we can have without the parser recognizing an unsized type, which is the same difficulty with omitting the keyword. Not sure.

BTW, just IMO, if we do keep a keyword, I'd suggest something like function or func rather than functions, just to make it clear that it's used before one function definition rather than a section.

[nhuurre]

Yeah, I guess printing nice error message is no easier. The potential errors I found are

";" or plain assignment is expected after a top-level variable declaration.

Identifier expected after type in top-level variable declaration.

Expected non-array type after array in return type.

and I think it's reasonable to append note: local functions must be declared with keyword to these. And yes, functions is not ideal, it was just a placeholder.

[rok-cesnovar]

I gave this a run, looks great. One minor thing missing is the +1 for start/end of reduce_sum.

[nhuurre]

New tarball: https://github.com/nhuurre/cmdstan/releases/download/closures-v0.2/cmdstan-closures-adjoint-ode.tar.gz

• The keyword is now function
• reduce_sum start/end indexes should be fixed
• integrate_1d and the experimental ode_adjoint_tol

[SteveBronder]

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

[nhuurre]

Just to check, this only does closures and not the lambda syntax

Yes, no lambda syntax. I mentioned the differences from the design doc here. (but that's outdated, the special suffixes _rng/_lpdf/_lp are now allowed on closures.)

[nhuurre]

can you give the test function names that line up with what they intend to check or validate?

I renamed the files and functions to something a bit more explanatory and added a unit test. The tests are admittedly rather haphazard.

[wds15]

I just tried this out and I really like it. While I got it working I was slightly irritated by two things:

• No semicolon after defining the closure.
• No need for a block "functions" within the modeling blocks.

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!

[nhuurre]

No semicolon after defining the closure.

Function definitions in the functions block don't have semicolons either. Since a lone semicolon is an allowed (no-op) statement it is not an error to put a semicolon after a closure definition. I think it would be a bit weird to require it though.

No need for a block "functions" within the modeling blocks.

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 functions block are available in all later blocks--so what exactly is the scope for a local functions block? I'm not opposed to the idea but it didn't seem to add much.

[bob-carpenter]

No semicolon after defining the closure.

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++.

[wds15]

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 functions block are available in all later blocks--so what exactly is the scope for a local functions block?

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.

[rok-cesnovar]

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 foo() to work in this case would be a bit confusing to me.

but I also still think a design doc would be great

A design doc for this was done in 2019 by @bob-carpenter: https://github.com/stan-dev/design-docs/pull/6 This PR deviates from the design doc in some minor implementation details, as Niko noted in a comment above: https://github.com/stan-dev/stanc3/pull/742#issuecomment-862544022

Once this PR is merged I think the important details should probably be added to the design doc so it reflects what was added.

[wds15]

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 on

This 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.

[WardBrian]

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