New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`increment_log_prob` only allowed in the `model` section #889

Closed
tpapp opened this Issue Aug 19, 2014 · 15 comments

Comments

Projects
None yet
6 participants
@tpapp
Copy link

tpapp commented Aug 19, 2014

Running

parameters {
  real<lower=0> y;
}
transformed parameters {
  real<lower=0> y_inv;
  y_inv <- 1 / y;
}
model {
  y ~ gamma(2,4);
}

from the manual using rstan (Version: 2.4.0, Date: 2014-07-20), I get the error message

TRANSLATING MODEL 'gamma' FROM Stan CODE TO C++ CODE NOW.
Error in stanc(file = file, model_code = model_code, model_name = model_name,  : 
  failed to parse Stan model 'gamma' with error message:
EXPECTATION FAILURE - DIAGNOSTIC(S) FROM PARSER:

sampling only allowed in model.


LOCATION OF PARSING ERROR (line = 8, position = 21):

PARSED:

transformed parameters {
  real<lower=0> y;
  y <- 1 / y_inv;
  // change
  increment_log_prob

EXPECTED: <eps> BUT FOUND: 

( -2 * log(y_inv) );
  // adjustment
}

Moving increment_log_prob to the model section resolves the issue, but the manual says it should be allowed in transformed parameters, too (and many examples have it there).

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

bob-carpenter commented Aug 19, 2014

Thanks. I thought we were still allowing increment_log_prob in
transformed parameters. I must have inadvertently tightened the
allowed contexts for increment_log_prob over using the lp__ variable.

I actually think that's the right thing to do, but I didn't mean
to just impose it on Stan without a discussion. The reason I like the
restriction is that it places all of the log probability affecting
statements in one block.

So the question is whether we should change the doc to match the current
behavior (increment_log_prob only in the model block) or whether
we should go back to allowing changes to lp__ in the transformed
parameters block.

  • Bob

P.S. It has the same effect on the log probability in the model as in
the transformed parameters, so there's no reduction in what can
be expressed.

On Aug 19, 2014, at 5:04 AM, tpapp notifications@github.com wrote:

Running

parameters {
real<lower=0> y;
}
transformed parameters {
real<lower=0> y_inv;
y_inv <- 1 / y;
}
model {
y ~ gamma(2,4);
}

from the manual using rstan (Version: 2.4.0, Date: 2014-07-20), I get the error message

TRANSLATING MODEL 'gamma' FROM Stan CODE TO C++ CODE NOW.
Error in stanc(file = file, model_code = model_code, model_name = model_name, :
failed to parse Stan model 'gamma' with error message:
EXPECTATION FAILURE - DIAGNOSTIC(S) FROM PARSER:

sampling only allowed in model.

LOCATION OF PARSING ERROR (line = 8, position = 21):

PARSED:

transformed parameters {
real<lower=0> y;
y <- 1 / y_inv;
// change
increment_log_prob

EXPECTED: BUT FOUND:

( -2 * log(y_inv) );
// adjustment
}

Moving increment_log_prob to the model section resolves the issue, but the manual says it should be allowed in transformed parameters, too (and many examples have it there).


Reply to this email directly or view it on GitHub.

@mbrubake

This comment has been minimized.

Copy link
Member

mbrubake commented Aug 19, 2014

I think it makes sense to allow increment_log_prob in transformed
parameters so that the log determinant corrections can be kept with the
transforms themselves. Forcing them to be separate seems likely to result
in errors where the corrections and the transforms aren't consistent.

On Tue, Aug 19, 2014 at 11:33 AM, Bob Carpenter notifications@github.com
wrote:

Thanks. I thought we were still allowing increment_log_prob in
transformed parameters. I must have inadvertently tightened the
allowed contexts for increment_log_prob over using the lp__ variable.

I actually think that's the right thing to do, but I didn't mean
to just impose it on Stan without a discussion. The reason I like the
restriction is that it places all of the log probability affecting
statements in one block.

So the question is whether we should change the doc to match the current
behavior (increment_log_prob only in the model block) or whether
we should go back to allowing changes to lp__ in the transformed
parameters block.

  • Bob

P.S. It has the same effect on the log probability in the model as in
the transformed parameters, so there's no reduction in what can
be expressed.

On Aug 19, 2014, at 5:04 AM, tpapp notifications@github.com wrote:

Running

parameters {
real<lower=0> y;
}
transformed parameters {
real<lower=0> y_inv;
y_inv <- 1 / y;
}
model {
y ~ gamma(2,4);
}

from the manual using rstan (Version: 2.4.0, Date: 2014-07-20), I get
the error message

TRANSLATING MODEL 'gamma' FROM Stan CODE TO C++ CODE NOW.
Error in stanc(file = file, model_code = model_code, model_name =
model_name, :
failed to parse Stan model 'gamma' with error message:
EXPECTATION FAILURE - DIAGNOSTIC(S) FROM PARSER:

sampling only allowed in model.

LOCATION OF PARSING ERROR (line = 8, position = 21):

PARSED:

transformed parameters {
real<lower=0> y;
y <- 1 / y_inv;
// change
increment_log_prob

EXPECTED: BUT FOUND:

( -2 * log(y_inv) );
// adjustment
}

Moving increment_log_prob to the model section resolves the issue, but
the manual says it should be allowed in transformed parameters, too (and
many examples have it there).


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#889 (comment).

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

bob-carpenter commented Aug 19, 2014

That was my original thinking for allowing them there.

The problem I now see is that we are going to want to
do things like separate models into base/delta (or
prior/likelihood if the terminological dust ever settles),
and we want to make sure everything affecting the log probability
goes into one or the other.

Which brings up what we do with the log prob increment from
transforms in this case --- is it a base or delta log prob?
We could potentially drop all the log prob statements in
transformed parameters into the same bucket with other transforms.

That way, they could be turned off for optimization, for example.

  • Bob

On Aug 19, 2014, at 11:39 AM, Marcus Brubaker notifications@github.com wrote:

I think it makes sense to allow increment_log_prob in transformed
parameters so that the log determinant corrections can be kept with the
transforms themselves. Forcing them to be separate seems likely to result
in errors where the corrections and the transforms aren't consistent.

On Tue, Aug 19, 2014 at 11:33 AM, Bob Carpenter notifications@github.com
wrote:

Thanks. I thought we were still allowing increment_log_prob in
transformed parameters. I must have inadvertently tightened the
allowed contexts for increment_log_prob over using the lp__ variable.

I actually think that's the right thing to do, but I didn't mean
to just impose it on Stan without a discussion. The reason I like the
restriction is that it places all of the log probability affecting
statements in one block.

So the question is whether we should change the doc to match the current
behavior (increment_log_prob only in the model block) or whether
we should go back to allowing changes to lp__ in the transformed
parameters block.

  • Bob

P.S. It has the same effect on the log probability in the model as in
the transformed parameters, so there's no reduction in what can
be expressed.

On Aug 19, 2014, at 5:04 AM, tpapp notifications@github.com wrote:

Running

parameters {
real<lower=0> y;
}
transformed parameters {
real<lower=0> y_inv;
y_inv <- 1 / y;
}
model {
y ~ gamma(2,4);
}

from the manual using rstan (Version: 2.4.0, Date: 2014-07-20), I get
the error message

TRANSLATING MODEL 'gamma' FROM Stan CODE TO C++ CODE NOW.
Error in stanc(file = file, model_code = model_code, model_name =
model_name, :
failed to parse Stan model 'gamma' with error message:
EXPECTATION FAILURE - DIAGNOSTIC(S) FROM PARSER:

sampling only allowed in model.

LOCATION OF PARSING ERROR (line = 8, position = 21):

PARSED:

transformed parameters {
real<lower=0> y;
y <- 1 / y_inv;
// change
increment_log_prob

EXPECTED: BUT FOUND:

( -2 * log(y_inv) );
// adjustment
}

Moving increment_log_prob to the model section resolves the issue, but
the manual says it should be allowed in transformed parameters, too (and
many examples have it there).


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#889 (comment).


Reply to this email directly or view it on GitHub.

@tpapp

This comment has been minimized.

Copy link

tpapp commented Aug 20, 2014

Conceptually, the only difference I can see between variables declared in the model and transformed parameters blocks is the former are recorded (write in Fig 23.1 of the manual), while the latter are not.

IMO it would make some sense to get rid of the transformed parameters block, and introduce some new semantics for the model block that would record variables.

Eg the qualifier keep below:

parameters {
  real<lower=0> y;
}
model {
  keep real<lower=0> y_inv;
  y_inv <- 1 / y;
  y ~ gamma(2,4);
}

The issue of separating the prior and the likelihood is orthogonal. I can imagine another qualifier, eg prior, that one could use to indicate sampling statements that belong in the prior.

(sorry, pressed wrong button and accidentally closed the issue, reopened again)

@tpapp tpapp closed this Aug 20, 2014

@tpapp tpapp reopened this Aug 20, 2014

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

bob-carpenter commented Aug 20, 2014

Andrew's said roughly the same thing in the past, and
emphasized that it's sometimes difficult to tell where to
define a variable, especially on the fly as you reparameterize
by editing an existing model file.

There are two bigger issues. One is that we check constraints
on the transformed parameters at the end of the transformed
parameter block. We don't currently allow constraints on local
variables because it's not clear when to test them for satisfying
the constraints.

The second issue is that we often want to treat log probability
introduced by the Jacobian separately because we might want to
do maximum likelihood estimation and be able to turn off the Jacobian.

  • Bob

On Aug 20, 2014, at 9:00 AM, tpapp notifications@github.com wrote:

Conceptually, the only difference I can see between variables declared in the model and transformed parameters blocks is the former are recorded (write in Fig 23.1 of the manual), while the latter are not.

IMO it would make some sense to get rid of the transformed parameters block, and introduce some new semantics for the model block that would record variables.

Eg the qualifier keep below:

parameters {
real<lower=0> y;
}
model {
keep real<lower=0> y_inv;
y_inv <- 1 / y;
y ~ gamma(2,4);
}

The issue of separating the prior and the likelihood is orthogonal. I can imagine another qualifier, eg prior, that one could use to indicate sampling statements that belong in the prior.


Reply to this email directly or view it on GitHub.

@syclik syclik modified the milestone: Future Oct 20, 2014

@tpapp

This comment has been minimized.

Copy link

tpapp commented Dec 6, 2014

@bob-carpenter: Regarding the Jacobian, maybe it would make sense to introduce an environment that only evaluates its contents when doing MCMC, not ML. Eg

parameters {
  real<lower=0> y;
...
model {
  log(y) ~ normal(mu,sigma);
  jacobian_adjustment {
    increment_log_prob(- log(y));
  }
...

though possibly with a more compact name. This would have the advantage of documenting the semantics automatically (though that is not necessary for trivial examples).

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

bob-carpenter commented Dec 6, 2014

That's a great idea. The language was designed before I realized
we need to turn off Jacobian adjustments for transforms in MLE.

How about just taking any log prob increment in the transformed
parameters to correspond to a Jacobian adjustment?

Then you'd have to write your program as:

transformed parameters {
real log_y;
...
log_y <- log(y);
...
increment_log_prob(-log(y)); // Jacobian adjustment
}
model {
log_y ~ normal(mu,sigma);
...

The increment_log_prob could go in a labeled sub-block to make it
clear it's in a Jacobian.

  • Bob

On Dec 6, 2014, at 4:09 AM, tpapp notifications@github.com wrote:

@bob-carpenter: Regarding the Jacobian, maybe it would make sense to introduce an environment that only evaluates its contents when doing MCMC, not ML. Eg

parameters {
real<lower=0> y;
...
model {
log(y) ~ normal(mu,sigma);
jacobian_adjustment {
increment_log_prob(- log(y));
}
...

though possibly with a more compact name. This would have the advantage of documenting the semantics automatically (though that is not necessary for trivial examples).


Reply to this email directly or view it on GitHub.

@ksvanhorn

This comment has been minimized.

Copy link
Contributor

ksvanhorn commented Nov 7, 2015

I'd like to note that the Stan compiler and the manual are still in disagreement on this issue. Section 25.2 of the Stan 2.8.0 Reference Manual states that

"Statements in the transformed parameter block and model block can have an effect on the log probability function defined by a model."

and

"The total log probability is initialized to zero. Then statements in the transformed parameter block and model block may add to it. The most direct way this is done is through the log probability increment statement,..."

whereas the Stan compiler gives an error if you use increment_log_prob in the transformed parameters block.

@tpapp

This comment has been minimized.

Copy link

tpapp commented Nov 8, 2015

Responding to the mention on the mailing list. Since I opened the original issue, I have been thinking about this. I think that there are two, almost orthogonal, issues here:

  1. what additions to the final log (posterior or likelihood) function is enabled for which mode (ML or Bayesian).
  2. what variables are recorded. Eg transformed parameters are. But sometimes it would make sense to save the posterior sample for some intermediate variables. Or even disable saving some parameters, if one wants to make the resulting file smaller.

(1) is about ~ (or equivalently incrementation) statements, while (2) is about variables.

IMO Stan should have semantics for designating this information in a stan file.

@bob-carpenter: I would rather dispense with the transformed parameters block altogether, and have everything in the model. The user would just declare variables, make transformations, and then designate that some variables are recorded, some aren't. By default all params would be recorded, and nothing else (except generated quantities). The user could change this for all variables, eg at the declaration:

parameters {
  real x discarded;
  real y; // kept by default
}
model {
  real z saved;
  real v; // discarded by default
}

Blocks like

jacobian_adj {
}
prior {
}

would designate the semantics of pieces of code.

@tpapp

This comment has been minimized.

Copy link

tpapp commented May 17, 2016

Is there any chance of fixing this issue (ie allowing increment_log_prob statements in the transformed_parameters block) in 2.10?

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

bob-carpenter commented May 17, 2016

2.10 is frozen awaiting the release, so no new features.

It should be a relatively easy patch because the
code runs in the same C++ context. I should be able to
get it out in 2.11.

Is the the motivation is that you have local variables
in the transformed parameters block that you want to include
in an increment_log_prob statement? If not, the code can just
move down to the model block.

First we need to see if it's something everyone thinks
is a good idea. As I've said before, it was my original
intent to allow it because it seemed the natural place to
put Jacobian adjustments.

  • Bob

On May 17, 2016, at 4:04 AM, Tamas K. Papp notifications@github.com wrote:

Is there any chance of fixing this issue (ie allowing increment_log_prob statements in the transformed_parameters block) in 2.10?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@tpapp

This comment has been minimized.

Copy link

tpapp commented May 18, 2016

The motivation is the following (very simplified): there is a parameter μ ∈ [0,1], which I am transforming to some α, which I need to record, and also they are on the LHS of some ~ statements (the parameter of interest is α, but transformations from μ are required because of censoring). However, during the transformation I calculate a large set of interim quantities, let's call them Λ, which are needed to increment the lp.

The cleanest, ideal solution would be defining a function mu2alpha_lp, which computes α from μ, and increments the lp at the same time, make α a transformed parameter, deal with corresponding ~ statements in the model block. This is currently ruled out by what I perceive as an arbitrary restriction.

The second best, which I am using currently, is have a mu2alphaLambda return an extra element in the vector it calculates, which is the sum of the log Λs. This I then extract in the model block, and increment lp accordingly. Code is somewhat ugly as a result.

My impression is that many nontrivial calculations follow this pattern: interim quantities during the transformation can be reused for Jacobian adjustments. A common pattern for me is transformations which involve fractions, eg when α = x/(x+μ) for some x (which is data), then I can reuse the denominator.

If I was not using functions, I could define an lp_kitchen_sink transformed parameter, accumulate all the Jacobian adjustments there, and use it in the model block in a single step. However, since functions in Stan cannot return multiple values and the variable would not be in scope for them, I would have to go through the same kind of acrobatics cutting up vectors do implement it.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

bob-carpenter commented May 18, 2016

Thanks for the overview. Yes, I imagine that's not an uncommon
pattern. We usually recommend making everything local in the model
block then recomputing as necessary in generated quantities, which is
roughly free computationally for a model of any complexity (no derivatives
and only computed once per iteration).

There's a more general feature request:

#1289

and also a more specific one that you created 2 year ago:

#889

I'll take a shot at getting these done soon as long as there
aren't convincing objections from the dev team.

  • Bob

On May 18, 2016, at 2:59 AM, Tamas K. Papp notifications@github.com wrote:

The motivation is the following (very simplified): there is a parameter μ ∈ [0,1], which I am transforming to some α, which I need to record, and also they are on the LHS of some ~ statements (the parameter of interest is α, but transformations from μ are required because of censoring). However, during the transformation I calculate a large set of interim quantities, let's call them Λ, which are needed to increment the lp.

The cleanest, ideal solution would be defining a function mu2alpha_lp, which computes α from μ, and increments the lp at the same time, make α a transformed parameter, deal with corresponding ~ statements in the model block. This is currently ruled out by what I perceive as an arbitrary restriction.

The second best, which I am using currently, is have a mu2alphaLambda return an extra element in the vector it calculates, which is the sum of the log Λs. This I then extract in the model block, and increment lp accordingly. Code is somewhat ugly as a result.

My impression is that many nontrivial calculations follow this pattern: interim quantities during the transformation can be reused for Jacobian adjustments. A common pattern for me is transformations which involve fractions, eg when α = x/(x+μ) for some x (which is data), then I can reuse the denominator.

If I was not using functions, I could define an lp_kitchen_sink transformed parameter, accumulate all the Jacobian adjustments there, and use it in the model block in a single step. However, since functions in Stan cannot return multiple values and the variable would not be in scope for them, I would have to go through the same kind of acrobatics cutting up vectors do implement it.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@sakrejda

This comment has been minimized.

Copy link
Contributor

sakrejda commented May 18, 2016

On Wed, May 18, 2016 at 2:19 PM Bob Carpenter

I'll take a shot at getting these done soon as long as there
aren't convincing objections from the dev team.

+1 for it being a great place to put jacobian adjustment. K

  • Bob

On May 18, 2016, at 2:59 AM, Tamas K. Papp notifications@github.com
wrote:

The motivation is the following (very simplified): there is a parameter
μ ∈ [0,1], which I am transforming to some α, which I need to record, and
also they are on the LHS of some ~ statements (the parameter of interest is
α, but transformations from μ are required because of censoring). However,
during the transformation I calculate a large set of interim quantities,
let's call them Λ, which are needed to increment the lp.

The cleanest, ideal solution would be defining a function mu2alpha_lp,
which computes α from μ, and increments the lp at the same time, make α a
transformed parameter, deal with corresponding ~ statements in the model
block. This is currently ruled out by what I perceive as an arbitrary
restriction.

The second best, which I am using currently, is have a mu2alphaLambda
return an extra element in the vector it calculates, which is the sum of
the log Λs. This I then extract in the model block, and increment lp
accordingly. Code is somewhat ugly as a result.

My impression is that many nontrivial calculations follow this pattern:
interim quantities during the transformation can be reused for Jacobian
adjustments. A common pattern for me is transformations which involve
fractions, eg when α = x/(x+μ) for some x (which is data), then I can reuse
the denominator.

If I was not using functions, I could define an lp_kitchen_sink
transformed parameter, accumulate all the Jacobian adjustments there, and
use it in the model block in a single step. However, since functions in
Stan cannot return multiple values and the variable would not be in scope
for them, I would have to go through the same kind of acrobatics cutting up
vectors do implement it.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#889 (comment)

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

bob-carpenter commented Nov 24, 2016

See #1289

@syclik syclik modified the milestones: Future, v2.13.0 Nov 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment