Closures

Closure spec.

You can post a rendered link here as well:
Rendered

To migrate some comments from the thread on Discourse here - you said there that you are proposing the capture-by-reference ([&] in C++) semantics for closures in this design doc, and that you weren't sure why we'd go with by-value. I still think it'd be good to have some discussion of the tradeoffs there in the doc, so I'll try to list some reasons I can think of to prefer capture-by-value:

I think many people would find capture-by-value less confusing, but I'm not sure about that. I certainly find immutability in a threaded context most appropriate, and this is the wisdom I have seen presented, e.g. in books like Java Concurrency in Practice, in the design of Clojure and Scala, in the implementations of most (all?) new parallelism frameworks recently such as Spark, Hadoop, Actor frameworks for various languages, Go channels, etc.
Semantics for threading will be hellacious if we don't make all variables in concurrent contexts immutable. In other words, we're opening up users to surprising race conditions in any parallel or concurrent context if we pass by reference and allow folks to mutate those captured variables within the lambda functions (which is what [&] allows in C++, if that's the proposed semantic). Making the referenced captures immutable within lambdas solves this issue but only in the context of threaded map_rect; if we start having other forms of concurrent programs it can become very confusing again.
Capture-by-reference semantics with mutable variables would add a source of non-determinism in a threaded context, making reproducibility harder.
We can pass these closures to threaded functions, but not to MPI, which destroys one of the major use-cases for lambdas with closures (a more user-friendly map_rect version).
We lose other immutability guarantees about Stan programs that we can use for optimization.

I claim we can have pass-by-value semantics with pass-by-reference performance in most cases by compiling down to closures with [&] wherever appropriate but creating copies if there's any chance the captured variable is mutated after the fact.
I'm a bit sleepy still - just to double-check, there aren't any additional considerations when creating a copy of a  var, right?

On Aug 9, 2019, at 3:31 AM, seantalts<EMAIL_ADDRESS>wrote:
To migrate some comments from the thread on Discourse here - you said there that you are proposing the capture-by-reference ([&] in C++) semantics for closures in this design doc, and that you weren't sure why we'd go with by-value. I still think it'd be good to have some discussion of the tradeoffs there in the doc,

Sure.  I literally couldn't imagine reasons we'd copy rather than use constant references.

so I'll try to list some reasons I can think of to prefer capture-by-value:
• I think many people would find capture-by-value less confusing, but I'm not sure about that. I certainly find immutability in a threaded context most appropriate, and this is the wisdom I have seen presented, e.g. in books like Java Concurrency in Practice, in the design of Clojure and Scala, in the implementations of most (all?) new parallelism frameworks recently such as Spark, Hadoop, Actor frameworks for various languages, Go channels, etc.

I was assuming we'd pass by constant reference.  I agree that adding mutable references to Stan will open a can of worms best left closed.
I don't think you can just write [const &] in C++ to enforce that at the C++ level.
I think all the rest of this is about immutability, not about reference vs. copy.

• Semantics for threading will be hellacious if we don't make all variables in concurrent contexts immutable. In other words, we're opening up users to surprising race conditions in any parallel or concurrent context if we pass by reference and allow folks to mutate those captured variables within the lambda functions (which is what [&] allows in C++, if that's the proposed semantic). Making the referenced captures immutable within lambdas solves this issue but only in the context of threaded map_rect; if we start having other forms of concurrent programs it can become very confusing again.

Agreed, let's make them immutable.

• Capture-by-reference semantics with mutable variables would add a source of non-determinism in a threaded context, making reproducibility harder.

Ditto.

• We can pass these closures to threaded functions, but not to MPI, which destroys one of the major use-cases for lambdas with closures (a more user-friendly map_rect version).

Is there a way to define the semantics for lambdas that lets them work neatly with MPI?  I was trying to call out the difficulty with all of our higher order functions:
integrate_ode:  needs to know variables vs. data for Jacobians
map_rect:  needs to know variables from data for storing data with instances and for knowing which derivatives to calculate
and presumably the algebraic solver will have similar issues.

• We lose other immutability guarantees about Stan programs that we can use for optimization.
I claim we can have pass-by-value semantics with pass-by-reference performance in most cases by compiling down to closures with [&] wherever appropriate but creating copies if there's any chance the captured variable is mutated after the fact.

I was trying to suggest the former.  Is there a case where you think allowing mutability of captured variables would be necessary?  It doesn't add any functionality as you could always just assign the immutable variable to a local (though it's clunkier).

I'm a bit sleepy still - just to double-check, there aren't any additional considerations when creating a copy of a var, right?

Copies of var work just like the var---they're just pointers to implementations (vari*).

I was always assuming for the parallelization things that the lambdas would have immutable references to its environment (like we are settling on). Anything else would be a problem.

Yeah, I didn't do a good job highlighting which issues occur due to lack of within-lambda immutability vs. lack of capture-by-value. If we assume within-lambda variable immutability (agreed it makes the most sense) we won't have issues with concurrency as long as it's confined to map_rect style functional concurrency. I prefer that style of concurrency and parallelism as far as user-defined constructs are concerned, but it does limit our ability to have the compiler automatically parallelize a Stan model. I think the only thing we lose is some performance in the cases where a user writes a model that constructs a local variable (data and params are immutable already) that they want to use in a lambda function and then modify after the lambda has finished executing. But we would only lose that performance if they are actually using (accidentally or on purpose) the fact that the variable changes values in between calls to the lambda (or if the compiler can't detect that they aren't and is forced to conservatively make a copy). Do you know if anyone would want to program models like that on purpose? It seems like a confusing set of semantics to me.
I think the MPI issue could be solved with a very similar set of compiler analyses that we'd need to have cheap capture-by-value semantics.
So far capture-by-value seems to still be coming out ahead given that

the implementations for both seem equivalently complicated, assuming we need to support map_rect
capture-by-value still seems less confusing and like what people (non-R people?) would naturally expect
capture-by-value allows for easier whole-program parallelization efforts, I think

I think either way we decide, it's good to add a section on capture-by-value vs. capture-by-reference. Would you like me to add that in?

Thanks, @bob-carpenter ! This is great. Sorry to have taken so long to get to it. I've been swamped with work lately. Happy to discuss on hangouts at any point!

+## Variable declarations
+
+Variable declarations for functions follows the usual syntax, with a
+sized type required for block variables, an unsized type for function
Does this mean you need a "sized function type" if you allow functions at the top level? What does that buy you over an unsized function type?

That was totally unclear and I had to think about what I could've meant.  I think I was just trying to stress that if you have something like an array of functions as a block variable, it needs to be sized
real(real)[2] fs = { (real x).x^2, (real x).log(x) };
but it could be unsized as a function argument, as in
real foo(real(real)[] fs, real x);

+## Lambda Expression Syntax
+
+Syntactically, a lambda expression is represented as a function argument
+list followed by a function body.   For example, in (real u) { return +1 / (1 + exp(-u)); }, the function argument list is (real u) and
Is it worth considering introducing a keyword for lambdas? For example \ or fun or function?

I'm totall OK thinking about alternative syntax for
types or lambdas.   I was just following the way it's done
in C++.
Are you suggesting something like
\real u { return u^2; }
or
function(real u) { return u^2; }
instead of
(real u) { return u^2; }  ?
I'm not so keen on backslash as it's a traditional escape.

I'm asking because the parentheses are already used for a lot of different things in Stan, as is, and probably will be used for more if we ever add tuples (unless we stick to structs instead). This is a genuine question. I'm not sure what I think, myself.

Are you imagining problems with parsing?

Bob

+## Implementation
+
+Lambdas can be mapped directly to C++ lambdas with default reference
+closures.  This will work because the scope of the compiled C++ is the
+same as that of the Stan program.
Is it worth thinking about an implementation strategy of compiling away the higher-order functions using something like defunctionalization?

Is that some way of unfolding a function like a macro?   If not, is there a simple description or reference somewhere?

My feeling is that the copies would be so bad that we should not go with by reference.

I think the sign got garbled here.

It also feels more consistent with ordinary function argument which are passed by constant reference. Again, I feel like the enclosing variables should be passed by constant reference.

Yes, which would mean they couldn't be changed in the closure.
The issue, as Sean pointed out, is that they might be changed out from under the closure.
x = 2;
int(int) f = (int y) { return x + y; }
int z1 = f(2);  // 4, as one would expect
x = 3;
int z2 = f(2);  // 5 if pass by reference, 4 if pass by value

+4.  Should full covariance and contravariance for function and array

types be required for assignment (including function calls)?

We don't have to consider this as part of the closures proposal, I think :) But I'm curious (and maybe we should take this to a github issue) - what all would be required? I guess if C++ didn't support automated casting we'd have to generate code that did the casting?

Not for array types, but we should sort it out and do the right thing for function assignment.
What would be required for array types is to basically allow assignment of int[] to double[],
and make that automatic in function calls.  I think that'd cover it.

Yes, which would mean they couldn't be changed in the closure. The issue, as Sean pointed out, is that they might be changed out from under the closure. x = 2; int(int) f = (int y) { return x + y; } int z1 = f(2); // 4, as one would expect x = 3; int z2 = f(2); // 5 if pass by reference, 4 if pass by value

Agreed. They could be changed out from under the closure. But that's also true in Python. (Not that Python should be our model for good language design, but at least it's a language that tons of people seem to find intuitive to use.) Generally, I don't love mutable variables at all. But given that Stan is already a language with mutable variables and given that function arguments are currently passed by constant reference, I would expect closures also to work by constant reference as those variables are basically implicit function arguments. (At least, that's often how you implement them, using something like lambda-lifting or closure conversion.)

I'm asking because the parentheses are already used for a lot of different things in Stan, as is, and probably will be used for more if we ever add tuples (unless we stick to structs instead). This is a genuine question. I'm not sure what I think, myself.
Are you imagining problems with parsing?

I don't think parsing would be a problem with the proposal as is, but it would become a problem if we ever want to move towards types being inferred more/being optional. I guess we could then let people write (auto x) {return x^2; } and it would still be OK.
Either way, I think we could get the formal parser to work with this. I was thinking more about keeping the language readable for people who aren't experts. In that respect, I'd like an informative keyword like function or fun or func. On the other hand, I can see that there might be an argument for brevity.

Is that some way of unfolding a function like a macro? If not, is there a simple description or reference somewhere?

It's a whole program transformation for replacing all function types with a kind of variant type and replacing all higher-order functions with first order functions. There's a concise description with examples here: http://spivey.oriel.ox.ac.uk/corner/Defunctionalization_(Programming_Languages) . I guess our problem is that this is only really easy to do in a language which already has support for variant types and tuples. You would compile away the higher-order types using the variant types and tuples and then I guess you can compile away those as well later (or give support for them in the runtime).  Seeing that we don't have variant types and tuples to start with, perhaps this is not the most suitable implementation strategy for us. That's sad though as it would keep the MIR simpler.

Yes, which would mean they couldn't be changed in the closure. The issue, as Sean pointed out, is that they might be changed out from under the closure. x = 2; int(int) f = (int y) { return x + y; } int z1 = f(2); // 4, as one would expect x = 3; int z2 = f(2); // 5 if pass by reference, 4 if pass by value
Agreed. They could be changed out from under the closure. But that's also true in Python. (Not that Python should be our model for good language design, but at least it's a language that tons of people seem to find intuitive to use.)

I'm OK either way.  I haven't been able to think of a reason we'd want to change a closed variable out from under a closure, but that doesn't mean there's not one.

Generally, I don't love mutable variables at all. But given that Stan is already a language with mutable variables and given that function arguments are currently passed by constant reference, I would expect closures also to work by constant reference as those variables are basically implicit function arguments. (At least, that's often how you implement them, using something like lambda-lifting or closure conversion.)

I don't know what those are.  In C++, const or not and reference or not are just declarations on the member variable of the closure (the functor being created by the lambda), which get set when it's created.

We could add both variant types and tuples.  I definitely want to add tuples and structs.  I can't think of much use for variant types in a statistical model, but that doesn't mean we can't have them under the hood if they make code generation and optimization easier.

On Aug 30, 2019, at 5:49 AM, Matthijs Vákár<EMAIL_ADDRESS>wrote:
Is that some way of unfolding a function like a macro? If not, is there a simple description or reference somewhere?
It's a whole program transformation for replacing all function types with a kind of variant type and replacing all higher-order functions with first order functions. There's a concise description with examples here: http://spivey.oriel.ox.ac.uk/corner/Defunctionalization_(Programming_Languages) . I guess our problem is that this is only really easy to do in a language which already has support for variant types and tuples. You would compile away the higher-order types using the variant types and tuples and then I guess you can compile away those as well later (or give support for them in the runtime). Seeing that we don't have variant types and tuples to start with, perhaps this is not the most suitable implementation strategy for us. That's sad though as it would keep the MIR simpler.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

I feel pretty strongly that we shouldn't add more mutability unless there's a good reason for it because of its impacts on parallelism and the increased difficulty of reasoning about a program. Once you have closures with mutable state you effectively have an object system, which obviously some folks have historically thought was useful but is much more complex to reason about. Many of OO's early champions have backtracked away from objects with mutable state (see: the Java concurrency book).
I talked with a few users at Stan Con and all of them agreed that letting a variable change out from under a closure would be really confusing. That matches my feelings about it. I think folks also find Python's behavior here subtle and unintuitive.

I would expect closures also to work by constant reference as those variables are basically implicit function arguments. (At least, that's often how you implement them, using something like lambda-lifting or closure conversion.)

I would guess a very small percentage of our users are thinking about Stan closures being lambda-lifted or closure converted :P. I think we can easily create capture-by-value semantics that are free in the case where people don't mutate in between calls to a closure, so for me the question comes down to what kind of semantics you want users to reason about. Capturing by reference makes using closures for concurrent/parallel code really difficult both for the compiler writer and, in my opinion, the user reasoning about the language.

I like the idea of adding a keyword for it. We could use fun so that when people want to google it, they get these results: https://www.google.com/search?q=stan+fun. I don't have a preference other than shorter is nicer.

it's also worth noting that if we have closures as first class values (as they are in this proposal) then any implementation is required to create a data structure to hold the captured state anyway, and that could be equally by value or by reference. But yeah, I also just think giving users a consistent simple mental model for how closures work will work out better than assuming they know how they might be implemented in compiler textbooks. For either version, we can hopefully mitigate confusion by printing a warning if someone tries to mutate a captured variable in between calls to a closure.

On Aug 30, 2019, at 2:43 PM, seantalts<EMAIL_ADDRESS>wrote:
I feel pretty strongly that we shouldn't add more mutability unless there's a good reason for it because of its impacts on parallelism and the increased difficulty of reasoning about a program. Once you have closures with mutable state you effectively have an object system, which obviously some folks have historically thought was useful but is much more complex to reason about. Many of OO's early champions have backtracked away from objects with mutable state (see: the Java concurrency book).

C++ is backing away from OO altogether.
Josh Bloch's Effective Java starts by urging you to try to
code objects that are constructed in a good state and immutable.
The concurrency book's a little bit more subtle in getting the
memory model book;  it's also super-scary as it does a good job
of laying out the dangers of multithreading.
I think the OO people who build GUIs still like MVC and other
mutable object patterns, but I haven't looked in on that world
in years.  I think it's how all the Javascript object models work
to drive modern web pages.  So don't count mutable objects out yet.
Not liking this mutability is why I don't like the fit objects
that R (and RStan) return.  Just check out the return object for
lm() or glm() in R or the fit object in Rstan using str().

I talked with a few users at Stan Con and all of them agreed that letting a variable change out from under a closure would be really confusing. That matches my feelings about it. I think folks also find Python's behavior here subtle and unintuitive.

I don't!  It's exactly what you'd expect capture by reference to do.
But then a lot of people find the whole idea of references and
pointers confusing.  I intentionally avoided making the Stan programmer
reason about references thinking it'd all be a lot easier to understand
if it worked like Java.

Bob

We can avoid copies in cases where people don't mutate the variables in question in between calls to the lambda. I would say that is 99% of the use cases I've seen.
I don't think people can tell that functions are applied with constant reference vs by value in Stan currently.

I set it up that way following the way Java works. But Java programmers
know they can modify the internals of objects they pass, whereas C++ is
a bit more nuanced.
The key is encapsulating the autodiff types, which really need to pass
the embedded vari*.  But that's all hidden from users.

Bob

On Aug 30, 2019, at 2:53 PM, seantalts<EMAIL_ADDRESS>wrote:
I like the idea of adding a keyword for it. We could use fun so that when people want to google it, they get these results: https://www.google.com/search?q=stan+fun. I don't have a preference other than shorter is nicer.

The winner by that criterion is just "f" if we need a non-empty
keyword and "" if we don't:
function(double x) { return x^2 * 3; }
fun(double x) { return x^2 * 3; }
f(double x) { return x^2 * 3; }
(double x) { return x^2 * 3; }
The tension is that more interpretable is better and shorter is better,
but they pull in opposite directions.

On Aug 30, 2019, at 3:10 PM, seantalts<EMAIL_ADDRESS>wrote:
it's also worth noting that if we have closures as first class values (as they are in this proposal) then any implementation is required to create a data structure to hold the captured state anyway,

That's at least how C++ does it---it creates a closure object.

and that could be equally by value or by reference. But yeah, I also just think giving users a consistent simple mental model for how closures work will work out better than assuming they know how they might be implemented in compiler textbooks.

I don't know how compiler textbooks think of this, but I understand
const/non-const and reference/value distinctions as implemented
in C++.
I think this is one of those one-way things where people who get
closures will be able to immediately comprehend pass-by-value.
I don't know what newbies will expect.

For either version, we can hopefully mitigate confusion by printing a warning if someone tries to mutate a captured variable in between calls to a closure.

Definitely.
I asked Lauren about this example in R.

y <- 5

f <- function(x) x * y

f(3)
[1] 15

y <- 10

f(3)

She guessed that the final f(3) would return 30 and said it'd be confusing if it returned 15.
That's only one data point, but maybe we should ask a few more potential users.  I'll ask Andrew.

Well that's because she knows R, right? What if  you gave the example in
Stan? What would she prefer Stan do?
I'm not even sure we can use closures for MPI if we do by reference, so
maybe that should be upweighted.
On Fri, Aug 30, 2019 at 16:10 Bob Carpenter<EMAIL_ADDRESS>wrote:

On Aug 30, 2019, at 3:10 PM, seantalts<EMAIL_ADDRESS>wrote:
it's also worth noting that if we have closures as first class values
(as they are in this proposal) then any implementation is required to
create a data structure to hold the captured state anyway,

That's at least how C++ does it---it creates a closure object.

and that could be equally by value or by reference. But yeah, I also
just think giving users a consistent simple mental model for how closures
work will work out better than assuming they know how they might be
implemented in compiler textbooks.

I don't know how compiler textbooks think of this, but I understand
const/non-const and reference/value distinctions as implemented
in C++.
I think this is one of those one-way things where people who get
closures will be able to immediately comprehend pass-by-value.
I don't know what newbies will expect.

For either version, we can hopefully mitigate confusion by printing a
warning if someone tries to mutate a captured variable in between calls to
a closure.

Definitely.
I asked Lauren about this example in R.

y <- 5

f <- function(x) x * y

f(3)
[1] 15

y <- 10

f(3)

She guessed that the final f(3) would return 30 and said it'd be confusing
if it returned 15.
That's only one data point, but maybe we should ask a few more potential
users. I'll ask Andrew.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/design-docs/pull/6?email_source=notifications&email_token=AAGET3BS572XTYXMBFHMTVDQHF5BXA5CNFSM4II6NOYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5SU54Q#issuecomment-526733042,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGET3EWCSJ6YZCPZLPG42DQHF5BXANCNFSM4II6NOYA
.

 But that doesn't mean we want it in Stan, right? I don't think Stan should be a copy of any existing 30 year old language just with HMC tacked onto it.

Nope. She didn't know that's what would happen, just thought it made the most sense.
I'm guessing most of our applied stats users won't know the edge-case behavior of resetting a captured variable, which is why I reached out to a few other people to ask.

But that doesn't mean we want it in Stan, right? I don't think Stan should be a copy of any existing 30 year old language just with HMC tacked onto it.

Given how many Stan users come to Stan from R or Python, it makes sense to take their expectations into consideration, if only in explaining how Stan doesn't work like those languages.
I get that we may not be able to swing pass-by-reference given our other constraints on parallelism.

Nope. She didn't know that's what would happen, just thought it made the most sense.

Now I'm really curious - does she also think dynamic lexical scoping makes the most sense?

I'm guessing most of our applied stats users won't know the edge-case behavior of resetting a captured variable, which is why I reached out to a few other people to ask.

That's a good idea, thanks. I talked with @wds15 and a few other people at StanCon who thought it didn't make sense to let it change in between calls, but perhaps it was the way I framed the question. After all, I don't think people will be doing this on purpose as functions with mutable state are kind of hairy to deal with - I think they'll be more likely to have this sneak up on them unexpectedly.

Given how many Stan users come to Stan from R or Python, it makes sense to take their expectations into consideration, if only in explaining how Stan doesn't work like those languages.

Definitely.

I think a lot of my intuition here around concurrency and closures was through conversations with Heather Miller about her work on Spores: https://infoscience.epfl.ch/record/191239 Here's a relevant bit:

I strongly feel we should go with an option that allows for concurrency and parallelism, and I'm afraid that might only be capture-by-value.

On Aug 30, 2019, at 4:30 PM, seantalts<EMAIL_ADDRESS>wrote:
Nope. She didn't know that's what would happen, just thought it made the most sense.
Now I'm really curious - does she also think dynamic lexical scoping makes the most sense?

C'mon.  Nobody could think that!  She thought non-local capture by closure felt like it would be confusing.  So I doubt she's leaning on dynamic lexical scoping.

I'm guessing most of our applied stats users won't know the edge-case behavior of resetting a captured variable, which is why I reached out to a few other people to ask.
That's a good idea, thanks. I talked with @wds15 and a few other people at StanCon who thought it didn't make sense to let it change in between calls, but perhaps it was the way I framed the question. After all, I don't think people will be doing this on purpose as functions with mutable state are kind of hairy to deal with - I think they'll be more likely to have this sneak up on them unexpectedly.

I suspect you're right.  Which is partly why I thought I'd ask.
I'm really OK doing this either by reference or value.  I'd just like to get  some feedback first to make sure we're not missing something that's obvious to someone else.

Which of those do you think might be a problem for us?   The only one that looks relevant is the second last---capture references to mutable objects leading to race conditions.
What I do see as a problem is passing var as those are intrinsically mutable.

On Aug 30, 2019, at 4:38 PM, seantalts<EMAIL_ADDRESS>wrote:
I think a lot of my intuition here around concurrency and closures was through conversations with Heather Miller about her work on Spores: https://infoscience.epfl.ch/record/191239 Here's a relevant bit:
I strongly feel we should go with an option that allows for concurrency and parallelism, and I'm afraid that might only be capture-by-value.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Are vars actually inherently mutable? Or do they just create global side effects whenever they're used? I was thinking it was the latter case and that the thread-local storage (and the scatter/gather MPI approach) dealt with those side effects...effectively. My model is that if you shard computation and keep track of the autodiff stacks for vars in each of those shards separately, that you can then recombine them as long as the shards were actually allowed to be computed separately in the first place (i.e. no dataflow dependencies between them). Does that sound right (if vague)?

Also just wanted to raise a point @bbbales2 made to me - we'll have to think carefully about how to implement closures for our current higher order functions. I think we'll need to do something similar to lambda lifting but with packing and unpacking some or all of the closed-over variables for both the ODE functions (which under the hood have a required signature of f(y, t0, packed_parameters) and MPI (requires f(packed_parameters) too I believe).

I'm not entirely sure

On Aug 31, 2019, at 3:35 PM, seantalts<EMAIL_ADDRESS>wrote:
Are vars actually inherently mutable? Or do they just create global side effects whenever they're used? I was thinking it was the latter case and that the thread-local storage (and the scatter/gather MPI approach) dealt with those side effects...effectively.

Here's what happens:
var x = 5;       // creates vari on stack and sets x.vi_ to point to it
var y = 6;       // creates second vari on stack and sets y.vi_ to point to it
x.vi_ == y.vi_   // false
y = x;           // sets y.vi_ to x.vi_
y.vi_ == x.vi_   // true
y += 4;          // creates new vari for (y + 4) and then sets y.vi_ to point to it
I'm not sure that counts as inherent or not or what counts as a side effect.
Once a vari gets created on the stack, it doesn't get modified until
the reverse pass when its adjoint values get set.

My model is that if you shard computation and keep track of the autodiff stacks for vars in each of those shards separately, that you can then recombine them as long as the shards were actually allowed to be computed separately in the first place (i.e. no dataflow dependencies between them). Does that sound right (if vague)?

I don't think anything in Stan now is modifying a single var from multiple threads outside of MPI threading.

I tried to point this out in the original spec, but didn't have a good solution.  It'll vary by higher-order function we have.
For MPI, we need to distinguish

data to send to shards (in integer and real arrays now)
shared parameters across shard
shard-specific parameters

For ODE solvers we need to distinguish

data (in integer and real arrays)
parameters so we can create coupled system

But anything that conceivably gets sent to a new computer is going to need to point to data there.  Looks like lambda lifting might work for that (had to read up on it on Wikipedia).
It'd be awesome if we could do this in a way that was transparent to the user.  I didn't even imagine it was possible, but maybe we have enough data flow analysis now to figure it out.

On Aug 31, 2019, at 5:18 PM, seantalts<EMAIL_ADDRESS>wrote:
Also just wanted to raise a point @bbbales2 made to me - we'll have to think carefully about how to implement closures for our current higher order functions. I think we'll need to do something similar to lambda lifting but with packing and unpacking some or all of the closed-over variables for both the ODE functions (which under the hood have a required signature of f(y, t0, packed_parameters) and MPI (requires f(packed_parameters) too I believe).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Probably good to keep track of the following email thread here:

On Tue, 3 Sep 2019 at 21:29, Sean Talts<EMAIL_ADDRESS>wrote:
an excellent concern! I think it should be really easy to 1) see if any locals closed over are mutated later and then 2) create a copy conservatively if so, emitting a warning. I think in most of these use cases folks are just going to be closing over immutable stuff anyway (data, tdata, params, tparams) and not even local variables, never mind locals that they'd mutate in between calls.
I expect people to basically be writing lambdas inline mostly too, so there's no copying needed in those cases either (until we get to some future where we're doing automatic parallelization).
Let me know if my assumption about 1 above is wrong, but it seems easy enough from my perspective having not worked with the monotone framework but done some similar analyses ad hoc in a previous life.
On Tue, Sep 3, 2019 at 3:15 PM Matthijs Vákár<EMAIL_ADDRESS>wrote:
I'd mostly be fine with that, I think. There doesn't currently seem to be a way for our users to know that normal function arguments are passed by constant reference rather than value. Generally, I prefer capture by value as it leads to less confusing side-effecting. My main problem is still performance. Would this not be shooting ourselves in the foot/ defeating the purpose of adding closures to the language with all the extra copying? I'd want us to have thought through the performance implications properly before settling on pass by value. If performance wouldn't be a problem, I'd be fine with it / might even prefer it.
On Tue, 3 Sep 2019 at 19:31, Sean Talts<EMAIL_ADDRESS>wrote:
hey,
I assume you won't have much time to look at closures either - do you mind if we go with capturing by value in that case? I'm motivated by my strong intuition that it will make future concurrency/parallelism easier, as well as being one less source for errors for new Stan programmers.
-Sean

My response is
It should indeed be easy to detect whether the variables are mutated after definition of the closure (and before using the closure). That could be done with a combination of a forward and a reverse dataflow analysis (rough sketch):

perform a forward dataflow analysis pass (union), storing for each node a set of triples (node_id_cd, c_name, var_name) where node_id_cd is the node label of the definition of a closure, c_name is the identifier (name) of the closure, and var_name is an identifier that c_name closes over (that can be calculated using a free variables utility function); here, we gen on closure definition and we kill on redefinition/declaration; call the results of this analysis A1;
now perform a reverse dataflow analysis pass (again, union), storing for each node a set of quadruples (node_id_cd, c_name, var_name, node_id_cu) where node_id_ca is the node label of a node where the closure c_name is applied and the rest are as before; we gen if a c_name is applied at a node while (node_id_cd, c_name, var_name) is in A1 at that node; we kill if c_name gets assigned to/declared; call the results A2;
if (node_id_cd, c_name, var_name, node_id_cu) is in A2 at a node node_id_va it means that the variable var_name is not allowed to be assigned at node node_id_va and if it is that we should issue a warning saying that we are redefining a variable var_name that is closed over in the definition of c_name which happened at node_id_cd which is going to affect the applications of c_name at the node node_id_cu (where we print the nodes as a source code location corresponding to them in the error message).

I'm sure this can be optimized in many ways, for example using Maps instead of Sets depending on the precise lookups we do a lot in the analysis, but this could be a general idea. Hope it's not too vague.

Is pass-by-value being driven by a fear users won't understand
pass-by-reference or because it won't play nicely with MPI and
multi-threading?
I agree with Sean in my guess about use cases, but I've been
surprised before by what users will do with new programming
tools.  If that guess is right, we could just restrict capture
to immutable variables.

Bob

On Sep 4, 2019, at 4:02 AM, Matthijs Vákár<EMAIL_ADDRESS>wrote:
Probably good to keep track of the following email thread here:
On Tue, 3 Sep 2019 at 21:29, Sean Talts<EMAIL_ADDRESS>wrote:
an excellent concern! I think it should be really easy to 1) see if any locals closed over are mutated later and then 2) create a copy conservatively if so, emitting a warning. I think in most of these use cases folks are just going to be closing over immutable stuff anyway (data, tdata, params, tparams) and not even local variables, never mind locals that they'd mutate in between calls.
I expect people to basically be writing lambdas inline mostly too, so there's no copying needed in those cases either (until we get to some future where we're doing automatic parallelization).
Let me know if my assumption about 1 above is wrong, but it seems easy enough from my perspective having not worked with the monotone framework but done some similar analyses ad hoc in a previous life.
On Tue, Sep 3, 2019 at 3:15 PM Matthijs Vákár<EMAIL_ADDRESS>wrote:
I'd mostly be fine with that, I think. There doesn't currently seem to be a way for our users to know that normal function arguments are passed by constant reference rather than value. Generally, I prefer capture by value as it leads to less confusing side-effecting. My main problem is still performance. Would this not be shooting ourselves in the foot/ defeating the purpose of adding closures to the language with all the extra copying? I'd want us to have thought through the performance implications properly before settling on pass by value. If performance wouldn't be a problem, I'd be fine with it / might even prefer it.
On Tue, 3 Sep 2019 at 19:31, Sean Talts<EMAIL_ADDRESS>wrote:
hey,
I assume you won't have much time to look at closures either - do you mind if we go with capturing by value in that case? I'm motivated by my strong intuition that it will make future concurrency/parallelism easier, as well as being one less source for errors for new Stan programmers.
-Sean
My response is
It should indeed be easy to detect whether the variables are mutated after definition of the closure (and before using the closure). That could be done with a combination of a forward and a reverse dataflow analysis (rough sketch):
• perform a forward dataflow analysis pass (union), storing for each node a set of triples (node_id_cd, c_name, var_name) where node_id_cd is the node label of the definition of a closure, c_name is the identifier (name) of the closure, and var_name is an identifier that c_name closes over (that can be calculated using a free variables utility function); here, we gen on closure definition and we kill on redefinition/declaration; call the results of this analysis A1;
• now perform a reverse dataflow analysis pass (again, union), storing for each node a set of quadruples (node_id_cd, c_name, var_name, node_id_cu) where node_id_ca is the node label of a node where the closure c_name is applied and the rest are as before; we gen if a c_name is applied at a node while (node_id_cd, c_name, var_name) is in A1 at that node; we kill if c_name gets assigned to/declared; call the results A2;
• if (node_id_cd, c_name, var_name, node_id_cu) is in A2 at a node node_id_va it means that the variable var_name is not allowed to be assigned at node node_id_va and if it is that we should issue a warning saying that we are redefining a variable var_name that is closed over in the definition of c_name which happened at node_id_cd which is going to affect the applications of c_name at the node node_id_cu (where we print the nodes as a source code location corresponding to them in the error message).
I'm sure this can be optimized in many ways, for example using Maps instead of Sets depending on the precise lookups we do a lot in the analysis, but this could be a general idea. Hope it's not too vague.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

What I'd prefer to do?  I'm OK going either way on this.
I think we're going to surprise a subset of users no matter what we do if we capture mutable local variables.  So I'm OK to only capture constants.  Will that also make dealing with the MPI stuff easier where we want to guarantee something's immutable when we pass it off as fixed data?

Yep, I think it makes MPI easier to think about as well. And I think this is a great first move because we can always extend this to capture local mutable variables later if we decide we need that without breaking backwards compatibility, whereas we couldn't change from by-value to by-reference without a major version bump. Let's go with that for now if that sounds good to you!

I think it makes MPI easier to think about as well.

I like this reasoning too.

Yes, OK with me.

Do you want to update this doc and we can get it merged? :)

Yes, I'll update the doc.

I tried revising, but ran into the problem that in a block like transformed data, the values are still mutable. So we could get something like this:
transformed data {
  real x = 12;
  real(real) h = (real u) { return u + x; };    // capture x by value
  x = 1;  // changing x is allowed before end of block
  print(h(5));  // should still print 17 because of capture by value

Is that what we want?

I updated the draft to reflect capture by value, and highlighted the issue that arises still with variables changing in blocks like transformed data or transformed parameters or generated quantities.

Thanks! That's a really good point. I have a couple related thoughts:

I think it might be true that most uses of closures will be in lambdas used directly in the model block, which would mean most users wouldn't notice the by-value-ness, I hope.
One way to teach this might be that closures can't refer to any variables defined locally in that block. That would mean that you couldn't create a closure in transformed data that referred to something that was, at that time, mutable. What do you think about that route? I find it a little easier to explain to myself than the "can only capture things that will be immutable, but if it's currently mutable it will be by value." Plus I think most users who would write a closure over a transformed data variable would probably be expecting the result from the end of the block...

How about print a compile-time warning if a variable is assigned after it is captured?

How about print a compile-time warning if a variable is assigned after it is captured?

If we go with a version that allows capturing mutable variables, we definitely want to do this.

I think most users who would write a closure over a transformed data variable would probably be expecting the result from the end of the block...

That'd be easy to implement with pass by constant reference.  But it'd be odd for uses that came before a value changed.
I was thinking that it'd be natural to define lambdas in the transformed data and parameters block that depend on variables in that block, so that they can be used later.  But that leaves undefined what happens in the intermediate stage before a block variable gets its final value.
In retrospect, it would've been nice to make block variables immutable.  That way, they'd need to be initialized with an initializer, but that could be a no-arg closure that could use earlier variables.  Even then, it'd be messy for intermediate local variables that you want to reuse, but not save.

We could just disallow it if we could figure this out in the compiler.  That also seems odd w.r.t. how closures are normally used.

On Sep 18, 2019, at 8:39 PM, seantalts<EMAIL_ADDRESS>wrote:
How about print a compile-time warning if a variable is assigned after it is captured?
If we go with a version that allows capturing mutable variables, we definitely want to do this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

We could just disallow it if we could figure this out in the compiler.  That also seems odd w.r.t. how closures are normally used.

What does "that" refer to here? changing the value of something one has closed over?

Yes, we could issue an error instead of a warning if we find a closed over variable being modified.

On Sep 19, 2019, at 3:05 PM, seantalts<EMAIL_ADDRESS>wrote:
We could just disallow it if we could figure this out in the compiler. That also seems odd w.r.t. how closures are normally used.
What does "that" refer to here? changing the value of something one has closed over?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

That's fine with me and doable. Then we get the easy pedagogical rule of "You can't close over anything you are going to change."
Does this mean folks can close over mutable local variables but not if they are going to mutate them after the closure or lambda is created? That seems ...fine to me too, I think.
Shall we go with that? I'm not picky here as long as closures aren't trying to capture mutating data.

Ping - @bob-carpenter which way would you like to go here?

The proposal is now in an inconsistent state.  I updated to reflect a decision to only capture block variables and make captured variables immutable.  The upshot is that we can no longer support something like currying, which I use as an example in the proposal:
real(vector) curry_mult(row_vector x) {
  return
    (vector y) {
      return mult(x, y);  // ILLEGAL CAPTURE OF NON-BLOCK VARIABLE x
    }
}

We could support this case as well as local variables if we implement closures using pass-by-value.  Using pass-by-reference, the variable x goes out of scope on the return.  In general, I'm imagining closures will be much more performant if they don't require copying, which will expand the memory footprint of the program.

I think there is a middle ground, which I think we were talking about at one point, where we only allow the use of variables that are immutable in closures. This would allow both block variables and function parameters. The downside is that then we do conceptually need full closure objects and to perform copies of the function parameters. (I'm glad we're not doing pass-by-constant-reference, that gets super confusing in these nested closures or closures defined inside other functions case).
That said, I think it's probably a good idea to go incrementally here. Let's get the simpler version out and get people playing around with it before we expand what we can reference inside a closure. Does that sound good? If so I think that just means merging this as-is :)

The first example is something like
(int x) { return (int y) { return x * y; } }
so that'll need to get cleaned up if we can't close over
local variables.

On Sep 30, 2019, at 5:18 PM, seantalts<EMAIL_ADDRESS>wrote:
I think there is a middle ground, which I think we were talking about at one point, where we only allow the use of variables that are immutable in closures. This would allow both block variables and function parameters. The downside is that then we do conceptually need full closure objects and to perform copies of the function parameters. (I'm glad we're not doing pass-by-constant-reference, that gets super confusing in these nested closures or closures defined inside other functions case).
That said, I think it's probably a good idea to go incrementally here. Let's get the simpler version out and get people playing around with it before we expand what we can reference inside a closure. Does that sound good? If so I think that just means merging this as-is :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Ah yeah. I thought that's what your last commit had done, woops. Do you have a strong opinion on removing currying vs. no capture of mutables vs. full capture by value? If so I'm happy to go with it; if not my opinion is to go with the simplest one first (removing currying; only capture block variables) and show that off to users like Sebastian. This assumes it will be less work to go with the simplest and it will be easy to add additional capabilities later, which I think is true.

I think there is a middle ground, which I think we were talking about at one point, where we only allow the use of variables that are immutable in closures.

I have a strong opinion that we need this.
I think the prime benefactors of closures are going to be easy specification of things for ODEs/parallelization.
Like check out the ODE function here: https://github.com/ntua-unit-of-control-and-informatics/Diazepam-case-study/blob/master/Stan univariate/diazepam_univariate.stan
The packing/unpacking is super intense, and that's not even that bad.
I don't want to lean on the model block structure given that that is already kind of awkward. For instance, if the thing I wanted captured by the closure was a function of parameters, it'd end up being a transformed parameter and end up in the output.
This is from Bob:

I updated to reflect a decision to only capture block variables and make captured variables immutable.

I interpreted the last thing as somehow mark things that are captured as captured so that later assignments throw errors.
Like this is allowed:
x = 5
f = () { return x; }

But this fails at compile time (x was once mutable but then became immutable once it was captured):
x = 5;
f = () { return x; }
x = 2;

I'm interpreting that differently from only allowing capture of things that are immutable.

For instance, if the thing I wanted captured by the closure was a function of parameters, it'd end up being a transformed parameter and end up in the output.

Is this a big issue? Is it not worth putting out a simple version first with this constraint? It seems about as expensive as the copies it would require under the hood at first glance...
If so, or if Bob prefers, we can go with the "set anything captured to be immutable" version and just pay for copies in cases where we think the reference isn't going to be around long enough.

Is this a big issue?

Maybe not, but I also don't want the closure implementation to lean on the block design we have now. I don't want an assumption like this baked into the cake.

Is it not worth putting out a simple version first with this constraint?

I don't know, I just wanted to say that this sound quite annoying. I don't know what this costs in terms of the compiler or in terms of implementation, but I'm pretty sure locals-not-capturing would frustrate me pretty quick once I go to code models.
Given closures are about making things more convenient (we can accomplish anything without closures that we can with), I wanna push back against requiring variables to be immutable before they are captured.

I wanna push back against requiring variables to be immutable before they are captured.

Is your preferred model that we throw an error if someone tries to change a variable that has been captured?
I'm not really sure how much more difficult it will make the implementation either. My criteria are still satisfied in both of those worlds - that we don't allow capturing variables that can then mutate in some other scope, and I think implementation will be easy enough for both so I'm happy either way.
Separately I do want to push back against thinking of a design doc as a perfect and final solution - it just has to represent an improvement and shouldn't serve to discourage future iterations and innovations, especially in a case like this where we're explicitly discussing leaving some work for the future.

Is your preferred model that we throw an error if someone tries to change a variable that has been captured?

Yeah, if that's what is convenient to do and there isn't some other flaw I'm not considering :P.

It's an option with some convenience :p.
What about if you weren't considering convenience?
On Mon, Sep 30, 2019 at 14:45 Ben Bales<EMAIL_ADDRESS>wrote:

Is your preferred model that we throw an error if someone tries to change
a variable that has been captured?
Yeah, if that's what is convenient to do and there isn't some other flaw
I'm not considering :P.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/design-docs/pull/6?email_source=notifications&email_token=AAGET3DUJDRAO7YXBFRLYBDQMJCNRA5CNFSM4II6NOYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD76VW2A#issuecomment-536697704,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGET3FPKNBUFLYOHQFZOI3QMJCNRANCNFSM4II6NOYA
.

What about if you weren't considering convenience?

I'm only really considering convenience here :P. The two things I know I want this for are making ODEs really easy to program (no packing/unpacking) and then parallel evaluation of log densities with the new parallel stuff Sebastian has been talking about (in which case it solves the packing/unpacking problem for me).

I'm only really considering convenience here :P. The two things I know I want this for are making ODEs really easy to program (no packing/unpacking) and then parallel evaluation of log densities with the new parallel stuff Sebastian has been talking about (in which case it solves the packing/unpacking problem for me).

Ohhh you meant Stan user programming convenience. That's the perspective I wanted - sorry, thought you were citing implementation convenience.

Well here I am talking about compiler convenience: https://github.com/stan-dev/design-docs/pull/6#issuecomment-536697704, but I answered your question in terms of user perspective cause I don't really know what's convenient or not language side :D.

On Sep 30, 2019, at 5:49 PM, seantalts<EMAIL_ADDRESS>wrote:
Ah yeah. I thought that's what your last commit had done, woops. Do you have a strong opinion on removing currying vs. no capture of mutables vs. full capture by value? If so I'm happy to go with it; if not my opinion is to go with the simplest one first (removing currying; only capture block variables) and show that off to users like Sebastian. This assumes it will be less work to go with the simplest and it will be easy to add additional capabilities later, which I think is true.

Sure.  Let's start simple if that's easy enough with the parser.  I'll revise the doc to reflect that.
The bigger deal's going to be figuring out how we can rewrite map_rect and integrate_ode to use closures.  I think that'll require more thought around how to specify the parameters if we capture them rather than packing/unpacking them.  We might want to think about that.  But then if this simple thing won't work for that, nothing will.

On Sep 30, 2019, at 6:51 PM, Ben Bales<EMAIL_ADDRESS>wrote:

...

I don't want to lean on the model block structure given that that is already kind of awkward. For instance, if the thing I wanted captured by the closure was a function of parameters, it'd end up being a transformed parameter and end up in the output.

That's the current plan.  If you need a function of parameters, it needs to be declared as a transformed parameter.  That'll guarantee that variables that are captured never go out of scope (though copying them also gives us that guarantee).

This is from Bob:
I updated to reflect a decision to only capture block variables and make captured variables immutable.
I interpreted the last thing as somehow mark things that are captured as captured so that later assignments throw errors.

Yes.  I'll clarify that was the intent.

Like this is allowed:
x = 5
f = () { return x; }
But this fails at compile time (x was once mutable but then became immutable once it was captured):
x = 5;
f = () { return x; }
x = 2;
I'm interpreting that differently from only allowing capture of things that are immutable.

I'll reword---the idea is rather that capturing a variable makes it immutable.

On Sep 30, 2019, at 8:05 PM, seantalts<EMAIL_ADDRESS>wrote:
For instance, if the thing I wanted captured by the closure was a function of parameters, it'd end up being a transformed parameter and end up in the output.
Is this a big issue? Is it not worth putting out a simple version first with this constraint? It seems about as expensive as the copies it would require under the hood at first glance...

It's not the creation cost---it's the output that's annoying.  We may be solving that problem separately in the interfaces going forward, but so far I think only RStan lets you filter what's output.

If so, or if Bob prefers, we can go with the "set anything captured to be immutable" version and just pay for copies in cases where we think the reference isn't going to be around long enough.

The idea was to do both---only allow capture of block variables, then make the block variable immutable after it's captured.

I'm pretty sympathetic to Ben's programming convenience argument - I think that makes sense for a direction to go in. I'm happy with that (allowing folks to capture anything, if it's not a block variable we copy it, anything captured is immutable after capture) or with Bob's last proposal (only capturing block variables, immutable after capture). Bob I think it's up to you, this is your design doc :) We could even make it talk about a two phase implementation if you want. As the reviewer for this I guess I'm happy with all of the options we're currently talking about so just let me know when it's time to do the final check and merge.
Just to quickly throw out another option - we could change the definition of what is allowed to "only capturing variables that will be live as long as the closure is alive." This is somewhat confusing to explain properly but it would allow the above stuff (capturing transformed data and marking it immutable post-capture) as well as defining and capturing variables in the model block (and marking immutable post-capture). This is because all variables defined in the model block are local and don't leave the model block scope.
Maybe we can talk this out today at the meeting - I don't feel strongly about any of these options but would like to get one of these routes merged soon.

I created the branch and wrote the first draft, but the project is jointly owned by everyone and I don't get the final say.  I specifically don't want to make the existence of a draft an argument against doing something alternative that's better.
All else being equal, I think it's going to be convenient to capture locals.  I'm even OK letting undefined behavior rest if we can't catch captures that go out of scope.  Anything involving scope is going to be complicated for most of our users, the majority of whom come from R which is the wild west with lexical dynamic scope and conditionals not opening scope blocks.
Capturing variables at the top-level model block won't go out of scope, but we can have local blocks in the model that will go out of scope.
model {
  real(real) f;
  if (theta > 0.5) {
    real j = 32;
    f = (real x) { return x + j; }
  } else {
    f = (real x) { return x^2; }
  }
  // j now out of scope, f broken

In general, we won't even be able to decide if f ever gets a definition if the block inside is complex enough.

We can track what is variables are live at the time of the declaration and
only allow those, right?
On Thu, Oct 3, 2019 at 09:49 Bob Carpenter<EMAIL_ADDRESS>wrote:

I created the branch and wrote the first draft, but the project is jointly
owned by everyone and I don't get the final say. I specifically don't want
to make the existence of a draft an argument against doing something
alternative that's better.
All else being equal, I think it's going to be convenient to capture
locals. I'm even OK letting undefined behavior rest if we can't catch
captures that go out of scope. Anything involving scope is going to be
complicated for most of our users, the majority of whom come from R which
is the wild west with lexical dynamic scope and conditionals not opening
scope blocks.
Capturing variables at the top-level model block won't go out of scope,
but we can have local blocks in the model that will go out of scope.
model {
real(real) f;
if (theta > 0.5) {
real j = 32;
f = (real x) { return x + j; }
} else {
f = (real x) { return x^2; }
}
// j now out of scope, f broken
In general, we won't even be able to decide if f ever gets a definition
if the block inside is complex enough.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/design-docs/pull/6?email_source=notifications&email_token=AAGET3G4YZOR4NJZV6NNWODQMXZ65A5CNFSM4II6NOYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAIINKA#issuecomment-537953960,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGET3DPSUO3BFUB25QB4E3QMXZ65ANCNFSM4II6NOYA
.

We can track what is variables are live at the time of the declaration and
only allow those, right?

For capture by value, yes.

@enetsee do you have any thoughts on this? Seems like we're between some rules intended to let folks capture things that we can nicely capture by reference versus just capturing anything they'd like and copying if necessary (and making anything captured immutable in the parent scope after capture).

I'm now worried that if we do have closures of locals, they'll still wind up being variable.
Consider:
real(int) f = (int x) { return (real y) { ... some expression involving closures of data ...}; };
Then we could have something like this:
for (i in 1:10)
map_rect(f(i), ...)  // 10 different f(i)
So one issue is how we're going to define whether something is constant and can thus be shipped off as data in a map-rect or integrate_ode call.  Even worse, these conditionals can branch on parameter values.

On Oct 5, 2019, at 8:40 PM, seantalts<EMAIL_ADDRESS>wrote:
@enetsee do you have any thoughts on this? Seems like we're between some rules intended to let folks capture things that we can nicely capture by reference versus just capturing anything they'd like and copying if necessary (and making anything captured immutable in the parent scope after capture).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

I'm not sure I totally get the example, but it seems like maybe we should just go with capture-by-value and try to have the compiler be smart and save copies where it can. That seems to avoid a lot of these really tricky situations, right?

The example's important.  Let's work backwards.  For MPI, we need to have fixed data that we can ship to the core that's going to do the work on it.  How do we maintain that with closures when the closures can close different pieces of data each trip through a program?
For example,
if (theta > 0)
  f = g;
else
  f = h;
map_rect(f, ...);
``

Now we don't know if `g` or `h` is going to be called in `map_rect`.  This wasn't a problem before because there were no function variables.  But even now (Stan 2.21), we could have something like:

for (i in 1:2)
map_rect(f, ..., std::vector{ i });

so that the integer data varies by loop instance.  What happens now in this case?

In the latter case I think it ships them at runtime. @wds15 can you confirm? I may be misremembering.
So for closures with values associated, in cases like that we'd need to essentially create first class closures that are runtime C++ objects with associated values and ship them over in the same way, right?

@bob-carpenter once you decide which way you'd like to go we can merge this design doc and get started on the ragged one :)

What's left to decide?
Now that I've finished std::complex for math, I plan to jump back into working on the language pretty much full time.

On Dec 16, 2019, at 1:26 PM, seantalts<EMAIL_ADDRESS>wrote:
@bob-carpenter once you decide which way you'd like to go we can merge this design doc and get started on the ragged one :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

The design of map_rect requires you to assign a C++ type to every call of map_rect. That type is expected to label uniquely the data going into the function. So the call as outlined above would be illegal. map_rect will ship the data upon the first call and then not any more. Maybe we need additional safety checks in the parser to avoid such a call?

the call as outlined above

There were two---in a loop for repeated calls or in a conditional for unpredictable calls.  Either way, do we need to ensure map_rect is only used from top-level scope (not nested in loops, conditionals, etc.) and that the function used is data?  What else do we need to ensure.
Whatever we do going forward, if the above are illegal now, we need to make that clear to users.  I hadn't realized they were illegal.

There were two---in a loop for repeated calls or in a conditional for unpredictable calls. Either way, do we need to ensure map_rect is only used from top-level scope (not nested in loops, conditionals, etc.) and that the function used is data? What else do we need to ensure.

I think that's it. One day we could make the compiler smarter and generate the code to ship the data itself, similar to the way that @rok-cesnovar recently did the same thing for GPUs. But for now perhaps enforcing those conditions with nice error messages is a good thing to add, agreed.

What's left to decide?

What kind of capturing semantics you want to use - capture-by-value, capture-by-reference, or one of the few intermediate ones we discussed.

We should discuss this separately...but I wonder how @rok-cesnovar solved the issues with allowing shipping the correct way. We need to put some more restrictions on map_rect, but allowing  it still to be called from within functions would be vital to keep my Stan programs alive at least.... but let's stop this side-track conversation here for now.
The closure stuff is something I am really looking forward to.

I thought the decision was to have pass-by-value semantics, but implement with pass-by-constant-reference wherever possible.

On Dec 17, 2019, at 11:47 AM, seantalts<EMAIL_ADDRESS>wrote:
There were two---in a loop for repeated calls or in a conditional for unpredictable calls. Either way, do we need to ensure map_rect is only used from top-level scope (not nested in loops, conditionals, etc.) and that the function used is data? What else do we need to ensure.
I think that's it. One day we could make the compiler smarter and generate the code to ship the data itself, similar to the way that @rok-cesnovar recently did the same thing for GPUs. But for now perhaps enforcing those conditions with nice error messages is a good thing to add, agreed.
What's left to decide?
What kind of capturing semantics you want to use - capture-by-value, capture-by-reference, or one of the few intermediate ones we discussed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

Okay great! I hadn't realized we were good to go with this. I'll approve & merge. Thanks everyone!


trying to send with more than 0.000001 zeroes, there is no error, the send button just does nothing

Issue description
When attempting to send a value with more than 0.000001 zeroes, there should be an error. You only get an error about sending 0.000001 iota, not 0.0000001. I know it won't let you send less than 1 Mi right now anyway.
Wallet version
0.3.1

wallet version (version number, commit, or branch):

System specification
MacOS Big Sur 11.2.3
Mac/Win/Linux (+Version and or Distro if possible)
Is it a repeatable issue?
Y
Steps To reproduce the issue
Explain how the maintainer can reproduce the issue.

Go to send, enter address.
Enter 0.0000001
Try to send -> no error

Expected behaviour
Some error like the one that says you can't send less than 1 Mi.

Actual behaviour
Send button has no reaction.
Errors
Paste any errors that you see from the Error Log in the Firefly menu or share screenshots of the issue.

This will be fixed in the next version. Please reopen if you face it still.


Lack of generalized notebook/function to run extraction through all plates

General question about this file: how does plate1 go through feature extraction? Would there be an opportunity to generalize this work from a single file somehow? Stated a different way: how might development here have differed were there 100 plates to process?
Originally posted by @d33bs in https://github.com/WayScience/NF1_SchwannCell_data/pull/38#discussion_r1089175458

This has been solved in #42 where this module and the CP analysis pipeline has been generalized and automated.


feature/update-docs

Update documentation in correction modules and reformatting of the API documentation.

I found a problem with the current structure of the modules that can easily lead to import loops. I have started by moving RSS and Cube to the data_container. There are a few extra bits of refactoring that are needed in order to fully clean the repo.

Pinging here @miguelglezb @angelrls @paranoya so you are aware. I will probably merge this into main very soon.


Use only one glucose measurement in json

In all the places I've found, openaps has standardised on reporting glucose levels in mg/dL
However, in https://github.com/openaps/openaps/blob/master/openaps/vendors/medtronic.py#L248-L249 it returns the data in the units specified by the user.
This means that all code that consumes the glucose targets needs to be able to handle two cases. Unfortunately, this could lead to dire consequences, where we try and get sugar levels down to 5.0 in the wrong scale. This sort of thing has happened before: http://www.wired.com/2010/11/1110mars-climate-observer-report/
I think we should standardise entirely on mg/dL in all presented/created/parsed json - and leave it up to tools to display the values differently on the UI if needs be. This moves it to the presentation layer, and means that problems like the above can be avoided.
https://github.com/loudnate/openaps-monitor/pull/24 is an example of this happening in the presentation layer. We can have a field that contains the user-preferred units in the settings json, for example, that would make this sort of thing easier.
If everyone is ok with this, I'd like to change the bg targets code to always return mg/dL

@bewest Not sure if yo think that this is a good idea. Let me know if I should refactor!

I think decocare should detect the units the pump is using and report those.
Openaps can have an option to switch units if needed; the units used should be passed with the data somehow.
A use can be created to "convert $xyz data in mmol to mg/dL" to help other uses/reports if needed, the most worrying thing here is that decocare is failing to detect the units used by the pump appropriately.


[HOLD for payment 2024-12-03] [Company cards] Navigate to some other existing feed after removing one

Coming from here
When user has 2 or more company card feeds and we remove one of them, make sure to navigate them and show them some other feed right after the removal. No particular order, just take the first one in the domain_member object
Issue OwnerCurrent Issue Owner: @bfitzexpensify

No need for b0

The PR is opened for the review.

BugZero Checklist:

[ ] [Contributor] Classify the bug:

Bug classification

Source of bug:

[ ] 1a. Result of the original design (eg. a case wasn't considered)
[ ] 1b. Mistake during implementation
[ ] 1c. Backend bug
[ ] 1z. Other:

Where bug was reported:

[ ] 2a. Reported on production
[ ] 2b. Reported on staging (deploy blocker)
[ ] 2c. Reported on both staging and production
[ ] 2d. Reported on a PR
[ ] 2z. Other:

Who reported the bug:

[ ] 3a. Expensify user
[ ] 3b. Expensify employee
[ ] 3c. Contributor
[ ] 3d. QA
[ ] 3z. Other:

[ ] [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.
Link to comment:

[ ] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.
Link to discussion:

[ ] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template

[ ] [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.
Link to issue:

Regression Test Proposal
Precondition:

Test:

Do we agree 👍 or 👎

@DylanDylann can you please add comments or link prs in all the checklist sections?

@mountiny

[Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

About this point, this is not a bug, when implementing this feature we already missed from the design doc

Ok, nice feel free to leave such quick note in the checklist in future

@bfitzexpensify lets please add this regression test

Regression steps proposed. @DylanDylann offer sent for payment.

No need payment here. Let's close this issue


TypeError: Cannot read property 'set' of undefined

I started on a fresh insiders opening a first folder, then added a second folder. Then I saved the workspace and hit refresh:
ERR Cannot read property 'set' of undefined: TypeError: Cannot read property 'set' of undefined
    at e.addChild (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:2409:129)
    at file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:4694:92
    at Array.forEach (native)
    at file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:4694:70
    at Object.g [as _notify] (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:146:729)
    at Object.enter (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:150:61)
    at n.Class.derive._creator._run (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:151:866)
    at n.Class.derive._creator._completed (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:151:307)
    at Object.g [as _notify] (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:146:729)
    at Object.enter (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:150:61)
    at n.Class.derive._oncancel._run (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:151:866)
    at n.Class.derive._oncancel._completed (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:151:307)
    at file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:2945:168
    at file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:2946:31
    at file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:265:204
    at Function.<anonymous> (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:2946:4)
    at file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:264:833
    at s (file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:264:364)
    at file:///Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:264:468
    at _combinedTickCallback (internal/process/next_tick.js:73:7)
    at process._tickCallback (internal/process/next_tick.js:104:9)

Seems to be somewhere here:

I tried to reproduce and cannot. I hope you know what you are doing with that change in the explorer model, I did not verify if that breaks anything...

@bpasero thanks, if it breaks something I think we will see in insiders. Though it can break only already broken beahvior (trying to add children to a non folder)


Avoid warnings in PHP 7.2 from count()

PHP 7.2 introduces a warning when you try to count something that isn't countable. Sometimes $this->_expectedArgs is null:
count(): Parameter must be an array or an object that implements Countable

/tmp/vendor/mockery/mockery/library/Mockery/Expectation.php:313
/tmp/vendor/mockery/mockery/library/Mockery/ExpectationDirector.php:173
/tmp/vendor/mockery/mockery/library/Mockery/ExpectationDirector.php:138
/tmp/vendor/mockery/mockery/library/Mockery/ExpectationDirector.php:91
/tmp/test.php:7

Coverage increased (+0.01%) to 73.418% when pulling b2f20b33942c2c84bc3b3b169098b448b3e66737 on duncan3dc:ensure-count-can-count into 6fdb61243844dc924071d3404bb23994ea0b6856 on padraic:0.9.