allow lp functions in transformed parameters

[nhuurre]

Prompted by issue #748
One line change because write_array() method already had lp_accum__ variable. The fact that it's not needed for anything other than _lp functions suggest that this may have been intented all along.

In any case, this is a backwards compatibility gotcha and per discussion on the forum something like this is needed.

Release notes

Allow calling lp functions in transformed parameters block.

Copyright and Licensing

Copyright holder: Niko Huurre

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

[rok-cesnovar]

We need to remove allowing _lupdf in _lp functions as well https://github.com/stan-dev/stanc3/pull/767/files#diff-ac78bfdef130e5ec4b7b3b245cddf78d3e7cb992ea1849fadafb2a70e248a0ffL263

[rok-cesnovar]

Hm. And this would also be allowed then:

functions {
  void smth_lp(real p) {
    p ~ std_normal();
  }  
}
parameters {
    real y;
}
transformed parameters {
    real r = smth_lp(2.0);
}
model {
    y ~ std_normal();
}

[nhuurre]

Good catch.
I'd like to keep target += *_lupdf as being interchangeable with a tilde statement but I suppose it's not going to work...

[rok-cesnovar]

If we are not allowing _lupdf then we also shouldn't allow the tilde statement right? So _lp works with target += _lpdf/_lpmf and nothing else.

I guess one possible solution is to check that a _lp function used in TP does not have _lupdf/tilde but that can get messy and its also pretty tricky.

[nhuurre]

If we are not allowing _lupdf then we also shouldn't allow the tilde statement right?

lupdf in a tilde statement is not a problem because write_array() never reads the value of the accumulator. Although the value may be inconsistent with what was computed in log_prob() this is not visible to the user.
EDIT: unless the _lp function invokes target(). Hiding this from the user is not possible...

I guess one possible solution is to check that a _lp function used in TP does not have _lupdf/tilde but that can get messy and its also pretty tricky.

Yeah, I don't think special restriction based call-site are a good idea.

[rok-cesnovar]

I am not sure I completely understand, so allow me another question.

Allowing _lupdf would allow:

functions {
  real lprob_lp(real x) {
    return normal_lupdf(x | 0, 1);
  }
}
parameters {
  real x;
}
transformed parameters {
  real ll = lprob_lp(x);
}
model {
  target += ll;
}

and not allowing this case is the reason we have lupdf restrictions.

With the tilde statement this would only be a problem if someone did this right?

functions {
  real lprob_lp(real x) {
    x ~ normal(0, 1);
    return target();
  }
}
parameters {
  real x;
}
transformed parameters {
  real ll = lprob_lp(x);
}
model {
  target += ll;
}

Which is what stanc2 allows.

[nhuurre]

Yes, that looks right. So the inconsistency with unnormalized distributions was present already in stanc2, it was just difficult to find.

[rok-cesnovar]

Yeah. So we either:

a) allow _lp in TP, restrict _lupdf and reveal back the difficult to find inconsistency which is present in stanc2
b) not allow _lp in TP and technically break backward compatibility (status quo)
c) do a check of whether the _lp is inconsistency-proof and allow/not allow based on that

All bad options :)

a) is I guess the least bad? Though b) is the only bug-free option here.

[nhuurre]

It's also possible to forbid lupdf and target()/get_lp() but allow target+= and tilde in _lp function body.

I'm not sure what is the use case for target(); the function can't rely on it having a predictable value anyway. If it's only printed immediately for debugging purposes and never stored then the inconsistency isn't too worrying.

[nhuurre]

Pushed a change to disallow _lupdfs in _lp functions. With that the only nondeterminism is confined in target()/get_lp() and that's probably fine?

[rok-cesnovar]

Agree.