expose_stan_functions fails on functions postfixed with `_lp`

[bgoodri]

Summary:

expose_stan_functions does not work with functions that modify target, which worked previously

Description:

If you try, there is a compiler error

Reproducible Steps:

expose_stan_functions this

functions {
  void foo_lp() {
    target += normal_lpdf(0.5 | 0, 1);
  }
}

Current Output:

In file included from file7d8f62dde969.cpp:1:
In file included from /usr/local/lib/R/site-library/rstan/include/exporter.h:1:
In file included from /usr/local/lib/R/site-library/Rcpp/include/RcppCommon.h:168:
In file included from /usr/local/lib/R/site-library/Rcpp/include/Rcpp/as.h:25:
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/internal/Exporter.h:31:28: error: no matching constructor for initialization of 'stan::math::accumulator<double>'
                    Exporter( SEXP x ) : t(x){}
                                         ^ ~
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/as.h:87:41: note: in instantiation of member function 'Rcpp::traits::Exporter<stan::math::accumulator<double> >::Exporter' requested here
            ::Rcpp::traits::Exporter<T> exporter(x);
                                        ^
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/as.h:152:26: note: in instantiation of function template specialization 'Rcpp::internal::as<stan::math::accumulator<double> >' requested here
        return internal::as<T>(x, typename traits::r_type_traits<T>::r_category());
                         ^
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/InputParameter.h:46:49: note: in instantiation of function template specialization 'Rcpp::as<stan::math::accumulator<double> >' requested here
        ReferenceInputParameter(SEXP x_) : obj( as<T>(x_) ){}
                                                ^
file7d8f62dde969.cpp:75:77: note: in instantiation of member function 'Rcpp::ReferenceInputParameter<stan::math::accumulator<double> >::ReferenceInputParameter' requested here
    Rcpp::traits::input_parameter< stan::math::accumulator<double>& >::type lp_accum__(lp_accum__SEXP);
                                                                            ^
/usr/local/lib/R/site-library/StanHeaders/include/stan/math/prim/mat/fun/accumulator.hpp:25:7: note: candidate constructor (the implicit copy constructor) not viable: cannot convert argument of incomplete type 'SEXP' (aka 'SEXPREC *') to 'const stan::math::accumulator<double>' for 1st argument
class accumulator {
      ^
/usr/local/lib/R/site-library/StanHeaders/include/stan/math/prim/mat/fun/accumulator.hpp:33:3: note: candidate constructor not viable: requires 0 arguments, but 1 was provided
  accumulator() : buf_() {}
  ^

and the relevant part of the generated C++ file is

void foo_lp(double& lp__, stan::math::accumulator<double>& lp_accum__, std::ostream* pstream__);
RcppExport SEXP sourceCpp_1_foo_lp(SEXP lp__SEXP, SEXP lp_accum__SEXP, SEXP pstream__SEXP) {
BEGIN_RCPP
    Rcpp::RNGScope rcpp_rngScope_gen;
    Rcpp::traits::input_parameter< double& >::type lp__(lp__SEXP);
    Rcpp::traits::input_parameter< stan::math::accumulator<double>& >::type lp_accum__(lp_accum__SEXP);
    Rcpp::traits::input_parameter< std::ostream* >::type pstream__(pstream__SEXP);
    foo_lp(lp__, lp_accum__, pstream__);
    return R_NilValue;
END_RCPP
}

Expected Output:

None

RStan Version:

develop (2.18) branch from GitHub

R Version:

3.5

Operating System:

Debian

[bgoodri]

@sakrejda I don't know how to fix this

[sakrejda]

Whoops, I'll take a look at it, we should have it increment a target variable, yeah?

[bgoodri]

The target variable should be passed in as an argument with a default of zero

On Sun, Aug 19, 2018 at 7:33 PM Krzysztof Sakrejda notifications@github.com wrote:

Whoops, I'll take a look at it, we should have it increment a target variable, yeah?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414163918, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqrA-kcZ8eM3jGe0SvDxHRukV01saks5uSfXlgaJpZM4WDJD5 .

[sakrejda]

Ok, I'll do it Monday

[sakrejda]

Should the target be incremented or should it just add to the return value?

[bgoodri]

I think incremented because those functions are often void

On Sun, Aug 19, 2018 at 9:01 PM Krzysztof Sakrejda notifications@github.com wrote:

Should the target be incremented or should it just add to the return value?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414170176, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqksGf_39lLgTu3WiJTX_PSwq6Q0Uks5uSgp1gaJpZM4WDJD5 .

[bob-carpenter]

Definitely incremented. The _lp functions increment the target value passed by reference.

[sakrejda]

This needs two things:

1) @syclik would it be ok to add a constructor to accumulator.hpp that created an accumulator by taking a pointer to a buffer (not ownership b/c R still deletes it at the end)? We need this so that it can be a proxy for an R vector.

2) I need to add to exporter.h in rstan so that it constructs the accumulator when it's required as an input parameter, probably adding

namespace Rcpp {
template <> stan::math::accumulator as(SEXP x) {
  // make the proxy accumulator with x's memory in its vector
};

...

 namespace traits {

   template <>
    struct input_parameter<stan::math::acumulator&> {
      typedef typename Rcpp::ConstReferenceInputParameter<stan::math::accumulator> type ;

    };

}
}

If Dan's on board with going this route I can make PR's, I thought I could do this without changes in math but we can't construct a new buffer and have it magically updated in R.

[syclik]

I'm not exactly sure how that would work with accumulator. (not saying it can't, I think I don't have enough context) It looks RAII-ish and I don't know how to make it both be RAII and non at the same time. Almost seems like it'd be easier to call a different class? (It's code... I realize it can be done.)

Do you have the flexibility to call something else where you're proposing changes? And how do you guarantee there's enough memory allocated?

On Mon, Aug 20, 2018 at 6:46 AM Krzysztof Sakrejda notifications@github.com wrote:

This needs two things:

1.

@syclik https://github.com/syclik would it be ok to add a constructor to accumulator.hpp https://github.com/stan-dev/math/blob/develop/stan/math/prim/mat/fun/accumulator.hpp that created an accumulator by taking a pointer to a buffer (not ownership b/c R still deletes it at the end)? We need this so that it can be a proxy for an R vector. 2.

I need to add to exporter.h https://github.com/stan-dev/rstan/blob/develop/rstan/rstan/inst/include/exporter.h in rstan so that it constructs the accumulator when it's required as an input parameter, probably adding

namespace Rcpp { template <> stan::math::accumulator as(SEXP x) { // make the proxy accumulator with x's memory in its vector };

...

namespace traits {

template <> struct input_parameter<stan::math::acumulator&> { typedef typename Rcpp::ConstReferenceInputParameter type ;

};

} }

If Dan's on board with going this route I can make PR's, I thought I could do this without changes in math but we can't construct a new buffer and have it magically updated in R.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414274726, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZ_FxawwY9CXVDVEIT0go-9t3Yl9xCWks5uSpNygaJpZM4WDJD5 .

[sakrejda]

I agree that it's awkward. I think I could derive from stan::math::accumulator in rstan and override everything we need. Now that I write this, I realize Rcpp is basically using the class as a tag so I'm not sure we even need to use the accumulator internally (although we still want something that acts like it).

It's a little more code but I don't think efficiency is the same kind of issue there (no autodiff to worry about) so it would be straightforward code.

[bgoodri]

Why does the code need to pass a stan::math::accumulator? Why can't it just pass double(0) and increment that?

[sakrejda]

The signature R gets is determined by what Stan generates for standalone functions so the approaches I describe just deal with that. A third option would be for stanc to generate a signature that takes a double& and plumb that through. One thing I have not checked is how the accumulator is expected to behave: would a double work?

[bgoodri]

The generated code has stuff like

lp_accum__.add(normal_log(0.5,0,1));

so we probably have to figure out how to work with an accumulator.

On Mon, Aug 20, 2018 at 10:03 AM Krzysztof Sakrejda < notifications@github.com> wrote:

The signature R gets is determined by what Stan generates for standalone functions so the approaches I describe just deal with that. A third option would be for stanc to generate a signature that takes a double& and plumb that through. One thing I have not checked is how the accumulator is expected to behave: would a double work?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414327529, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrql8S6TW9x58vLs7ISRdfcwfJ-FRHks5uSsHLgaJpZM4WDJD5 .

[sakrejda]

That's what I guess, do you see any reason we can't derive from stan::math::accumulator and override the .add method?

[bgoodri]

Not that I know of. But can't we just put something into expose_stan_functions_hacks() to get around it, like deleting the accumulator and changing lp_accum.add to lp += ?

On Mon, Aug 20, 2018 at 10:27 AM Krzysztof Sakrejda < notifications@github.com> wrote:

That's what I guess, do you see any reason we can't derive from stan::math::accumulator and override the .add method?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414335268, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqrbrohZV_UWjZ6DhTayIEHCsKvwIks5uSsdNgaJpZM4WDJD5 .

[bgoodri]

Or, probably better, why doesn't the wrapper function construct a stan::math::accumulator internally and pass that to the function inside the model's namespace?

On Mon, Aug 20, 2018 at 10:42 AM Ben Goodrich goodrich.ben@gmail.com wrote:

Not that I know of. But can't we just put something into expose_stan_functions_hacks() to get around it, like deleting the accumulator and changing lp_accum.add to lp += ?

On Mon, Aug 20, 2018 at 10:27 AM Krzysztof Sakrejda < notifications@github.com> wrote:

That's what I guess, do you see any reason we can't derive from stan::math::accumulator and override the .add method?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414335268, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqrbrohZV_UWjZ6DhTayIEHCsKvwIks5uSsdNgaJpZM4WDJD5 .

[sakrejda]

I'm inclined to avoid hacks because there are plenty of clean ways to do this that will generalize to python. It's just a question of which one to pick. If you need this for a release now more hacks is fine but I'm not sure easier. I could also expose the accumulator the way we did the rng so it could just be passed in, same kind a code would go into exporter.h

[bgoodri]

If the main concern is adding it to Python, then I think definitely the way to go would be to have the wrapper functions construct the things that need to be passed to the function inside the model's namespace but do not have a native representation in the interface.

On Mon, Aug 20, 2018 at 11:08 AM Krzysztof Sakrejda < notifications@github.com> wrote:

I'm inclined to avoid hacks because there are plenty of clean ways to do this that will generalize to python. It's just a question of which one to pick. If you need this for a release now more hacks is fine but I'm not sure easier. I could also expose the accumulator the way we did the rng so it could just be passed in, same kind a code would go into exporter.h

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414350713, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqhyxjNDFVsjJraju6rF-EiNwWLF7ks5uStDogaJpZM4WDJD5 .

[bob-carpenter]

I think @bgoodri's proposal to instantiate a stan::math::accumulator<double> is a clean approach.

Another option would be to add += operator implementations for accumulators on LHS and rewrite the code that uses accumulators to use += instead of add. That way, a double& instantiation would work.

A third option would be a custom implementation of the accumulator interface based just on a double reference.

But what @bgoodri is suggesting still seems easiest.

[sakrejda]

Ok, will do, it's exactly what we did with the RNG and the R/Rcpp get_rng functions.

[sakrejda]

@bgoodri I made a branch but I haven't gotten through re-installing StanHeaders/rstan so I'll check that tomorrow and fix whatever bugs there are.

[bgoodri]

What is the benefit to having the accumulator exposed to R? What can you do with it besides pass it to a user-defined function ending in _lp?

On Mon, Aug 20, 2018 at 8:42 PM Krzysztof Sakrejda notifications@github.com wrote:

@bgoodri https://github.com/bgoodri I made a branch https://github.com/stan-dev/rstan/tree/bugfix/issue-550-postfix-lp but I haven't gotten through re-installing StanHeaders/rstan so I'll check that tomorrow and fix whatever bugs there are.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414510651, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqicYWBjGUMghN6jPK01eVw9OBiqRks5uS1dzgaJpZM4WDJD5 .

[sakrejda]

I missed adding that part. The only other thing you want to be able to do is check it's value. It has to live in R memory somehow (as a ptr is easiest) s.t. its value can be accessed.

[sakrejda]

It's the same as RNG but we want to be able to check its value so we can see the result of the function.

[sakrejda]

With the current code on the branch I get:

> 
> library(rstan)
> check_accumulator(ACC)
[1] 0
> expose_stan_functions('model.stan')
> foo_lp(0, ACC, OUT)
> check_accumulator(ACC)
[1] -1.043939
> 
> acc = get_accumulator()
> check_accumulator(acc)
[1] 0
> foo_lp(0, acc, OUT)
> check_accumulator(acc)
[1] -1.043939

So... that seems good enough for a bugfix. I'm realizing none of these have much roxygen doc. I can add that this weekend. They could probably do with explicit classes and print methods (like the print on an accumulator should just dump out the value)

[bgoodri]

Can't the function just return target() ?

On Tue, Aug 21, 2018 at 6:01 AM Krzysztof Sakrejda notifications@github.com wrote:

It's the same as RNG but we want to be able to check its value so we can see the result of the function.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414621255, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqthOKMBjVEYz4z5clqaiMCmZeDxtks5uS9p9gaJpZM4WDJD5 .

[sakrejda]

I don't know what "target()" is so I probably missed something?

[sakrejda]

Part of the problem is that I don't use the _lp functions much so I don't know what you expect that syntax to do in rstan.

[bgoodri]

In the Stan function, you can do

real foo_lp(...) { target += something; return target(); }

and it yields the accumulated value.

On Tue, Aug 21, 2018 at 12:24 PM Krzysztof Sakrejda < notifications@github.com> wrote:

I don't know what "target()" is so I probably missed something?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414735464, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqrQDPdsf4FhJITIZdJrcZ9xuJQxHks5uTDRYgaJpZM4WDJD5 .

[sakrejda]

I see, that's yet another signature, it should work with the current exporter.h code, did you try one?

[sakrejda]

I'm on phone rn

[sakrejda]

Longer term we could change how standalone functions are generated in stan-dev/stan so that they always return target and don't create an accumulator at all. We already do some special stuff for them.

[bgoodri]

I haven't tried your way yet. But what @bob-carpenter and I were saying is that the C++ wrapper could pass a stan::math::accumulator to the function in the model's namespace and then we don't have to expose anything on the interface side that is not usable in the interface. That is how I fixed it yesterday in https://github.com/stan-dev/rstan/commit/dabdbfd9bbfe524ce7c0854aecd7c0efa53fc901

[sakrejda]

That's fine, but why not tell me yesterday you'd fixed it? Did I miss something? I only fixed it because you asked and gsub-on-code is not an approach I want to maintain. When rstan did more grep/gsub previously I kept having failures when I was trying to get my own models done!

[bgoodri]

Well, I think for 2.19 we should have the parser generate the code we want so that we don't have to gsub (as much). But if everyone agrees that we should construct the stan::math::accumulator on the inside, then it would be a (minor) problem to introduce the accumulator in the interfaces in 2.18 and then take it away it 2.19.

On Tue, Aug 21, 2018 at 1:32 PM Krzysztof Sakrejda notifications@github.com wrote:

That's fine, but why not tell me yesterday you'd fixed it? Did I miss something? I only fixed it because you asked and gsub-on-code is not an approach I want to maintain. When rstan did more grep/gsub previously I kept having failures when I was trying to get my own models done!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414757429, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqkLVguHqWouICIOASoROSJ2iZZcPks5uTERKgaJpZM4WDJD5 .

[sakrejda]

Ok, that's fine with me. Might want to check if the changes we're expecting to make in the parser are plausible.

[bgoodri]

Plausible @bob-carpenter ?

[bob-carpenter]

I'm not sure which of the topics you're asking about.

Did you want some kind of change to the parser?

On Aug 21, 2018, at 10:47 PM, bgoodri notifications@github.com wrote:

Plausible @bob-carpenter ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[bgoodri]

Having the parser generate

void
foo_lp(double& lp__, std::ostream* pstream__ = nullptr){
stan::math::accumulator<double> lp_accum__; // this line instead of it
being part of the signature
model_namespace::foo_lp<double, stan::math::accumulator<double> >(lp__,
lp_accum__, pstream__);
}

from

functions {
  void foo_lp() {
    target += normal_lpdf(0.5 | 0, 1);
  }
}

On Tue, Aug 21, 2018 at 5:09 PM Bob Carpenter notifications@github.com wrote:

I'm not sure which of the topics you're asking about.

Did you want some kind of change to the parser?

On Aug 21, 2018, at 10:47 PM, bgoodri notifications@github.com wrote:

Plausible @bob-carpenter ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414822519, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqsaJPbM3rhSRr1xCokwL2oYfC0-Iks5uTHbzgaJpZM4WDJD5 .

[sakrejda]

And also always adding return target();, right?

[bgoodri]

I think that should be up to the user.

On Tue, Aug 21, 2018 at 5:38 PM Krzysztof Sakrejda notifications@github.com wrote:

And also always adding return target();, right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414830326, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqltX2pV34egbncJYSMOOUYqjYzyEks5uTH3kgaJpZM4WDJD5 .

[sakrejda]

That would mean you couldn't test functions that just rely on the accumulator from R whereas with my approach they both work.

[bgoodri]

If you want to test them or use the value from R, then you just return target(); We do that a bunch in rstanarm.

On Tue, Aug 21, 2018 at 5:49 PM Krzysztof Sakrejda notifications@github.com wrote:

That would mean you couldn't test functions that just rely on the accumulator from R whereas with my approach they both work.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-414833193, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqm64-BJu-8WeGoYyZc8HxInsmlkDks5uTIBggaJpZM4WDJD5 .

[sakrejda]

Functions might return something else already and it's error-prone to edit a bunch of functions to test them so I don't like that answer. This is your show so ¯_(ツ)_/¯

[bgoodri]

This is breaking a bunch of tests. If you call multiple functions ending in _lp (or the same function more than once), you get the accumulated value of target() on the second call unless you call it with lp_accum__ = get_accumulator().

[sakrejda]

Ok, I missed that in the test interface. That's expected behavior since with an accumulator all that's guaranteed is the increment. That's fixable, and I'll take a look at setting default parameter values so this all disappears from the user's perspective.

[bgoodri]

Do you mean a default value for get_accumulator that the accumulator is set to?

[sakrejda]

Yes, unfortunately it probably involves a PR to Rcpp since they currently hard code default value parsing.

[bgoodri]

Well, I am not sure how open they would be to that. But we have to gsub the C++ code in the meantime.

I don't think the idea of an accumulator even makes much sense outside the log_prob context, much less in the R or Python context.

On Thu, Aug 23, 2018 at 5:15 PM Krzysztof Sakrejda notifications@github.com wrote:

Yes, unfortunately it probably involves a PR to Rcpp since they currently hard code default value parsing.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/550#issuecomment-415573601, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqo_kWeEcl_9_gn6vq5czezpQplGVks5uTxuLgaJpZM4WDJD5 .

[sakrejda]

The current pr works without any more gsub or default values, you'd just have to pass all the requires args Stan requires