alashworth / test-issue-import

0 stars 0 forks source link

selective suppression increment_log_prob warning #104

Open alashworth opened 5 years ago

alashworth commented 5 years ago

Issue by sdewaele Tuesday Jul 26, 2016 at 10:49 GMT Originally opened as https://github.com/stan-dev/stan/issues/1978


Summary:

Introduce a tag in the comments of a line to selectively suppress the parser warning to call increment_log_prob.

Description:

The Stan parser issues a the following warning if the left-hand side of a sampling statement may contain a non-linear transform of a parameter or local variable. After performing this check, the programmer should be able add a tag to that line to suppress the warning in the future. This reduces unnecessary parser output, allowing the programmer to pay more attention to remaining, vaild warnings. Also, in this way a perfect program compiles without errors, which is also desirable.

For example, the syntax for this tag can be similar to Matlab's syntax for warning suppression.

Reproducible Steps:

The parser generates the warning during compilation of the following script:

data {
    real mu;
    real sigma;
}

parameters {
    real a;
    real b;
}

transformed parameters {
    real s;
    real bsq;
    s <- a + b;
    bsq <- b^2; 
}

model {
    s ~ normal(mu, sigma); # %ok<NOJACOBIAN>
    bsq ~ lognormal(3, 2);
}

Current Output:

DIAGNOSTIC(S) FROM PARSER:
Warning (non-fatal):
Left-hand side of sampling statement (~) may contain a non-linear transform of a parameter or local variable.
If so, you need to call increment_log_prob() with the log absolute determinant of the Jacobian of the transform.
Left-hand-side of sampling statement:
    s ~ normal(...)
Warning (non-fatal):
Left-hand side of sampling statement (~) may contain a non-linear transform of a parameter or local variable.
If so, you need to call increment_log_prob() with the log absolute determinant of the Jacobian of the transform.
Left-hand-side of sampling statement:
    bsq ~ lognormal(...)

Expected Output:

The first warning is suppressed because the user has checked the line and added the tag %ok<NOJACOBIAN> in the comments of the indicated line:

DIAGNOSTIC(S) FROM PARSER:
Warning (non-fatal):
Left-hand side of sampling statement (~) may contain a non-linear transform of a parameter or local variable.
If so, you need to call increment_log_prob() with the log absolute determinant of the Jacobian of the transform.
Left-hand-side of sampling statement:
    bsq ~ lognormal(...)

Current Version:

v2.9.0

alashworth commented 5 years ago

Comment by syclik Wednesday Jul 27, 2016 at 20:26 GMT


@sdewaele: thanks for providing a concrete suggestion. It's very helpful seeing an example and how it would play out.

Since we don't have annotation to the Stan language yet, adding annotation-based exceptions to warnings may be a little more difficult to do immediately, but we'll discuss it at a future meeting. (I don't want to get your hopes up -- it seems like a good idea, but it may be a while before we're able to get to it.)

alashworth commented 5 years ago

Comment by sdewaele Thursday Jul 28, 2016 at 17:18 GMT


Thanks for your reply! I hope it will eventually find its way into the language.

By the way, a quick workaround to suppress the warning, which is replace the line: s ~ normal(mu, sigma); by the equivalent increment_log_prob call: increment_log_prob(normal_log(s, mu, sigma)); However, I do think that the original line is more expressive, and expressiveness is what I hope to get out of Stan!

As a warning to this approach, I would like to point out that the warning message in the statement: bsq ~ lognormal(3, 2); can be suppressed in the same way. However, here the warning is actually correct, and the determinant should be added.

alashworth commented 5 years ago

Comment by bob-carpenter Friday Jul 29, 2016 at 19:30 GMT


Those aren't strictly equivalent, though they have the same effect on sampling and optimization.

increment_log_prob(u) is deprecated (still available, but spits out its own warnings) in favor of

  target += u

We are going to be moving some of these lint-like messages to a separate package and add more of them, so we've already been considering how to supress them. The cpplint package uses

.... // NOLINT(type)

where type is the type of error to suppress. We may just follow that convention.

On Jul 28, 2016, at 1:18 PM, sdewaele notifications@github.com wrote:

Thanks for your reply! I hope it will eventually find its way into the language.

By the way, a quick workaround to suppress the warning, which is replace the line: s ~ normal(mu, sigma); by the equivalent increment_log_prob call: increment_log_prob(normal_log(s, mu, sigma)); However, I do think that the original line is more expressive, and expressiveness is what I hope to get out of Stan!

As a warning to this approach, I would like to point out that the warning message in the statement: bsq ~ lognormal(3, 2); can be suppressed in the same way. However, here the warning is actually correct, and the determinant should be added.

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

alashworth commented 5 years ago

Comment by sdewaele Saturday Jul 30, 2016 at 14:43 GMT


Looking forward to having that option. I used the Matlab syntax as an example, the cpplint syntax works just as well.