Test speed advantage of using ulpdf instead of lpdf

[avehtari]

brms uses always target += instead of ~. The tilde version uses log unnormalized pdf to avoid computation of constants that don't affect sampling. brms uses target += with *_lpdf that are log normalized pdfs to support marginal likelihood computations, but then computes the constants also when not needed. The new Stan release 2.25 has support for *_lupdf that provide the log unnormalized pdf, so it would be possible to provide an option switch normalization off. It would be useful to make some speed tests with models that have expensive normalization terms and especially with for loops so that the normalization terms are computed many times. Unlikely of order of magnitude speed-ups but can be significant speed-ups for some models.

[wds15]

Test poisson and you should see a difference (and also dropping those ccdf calls should help) in speed.

[andrjohns]

Correct me if I'm wrong, but doesn't brms need to keep the _lpdf construction for compatibility with bridgesampling post-processing? Since that needs the normalising constants?

[andrjohns]

As soon I hit comment I saw the part of the issue that mentioned the option to switch normalising on/off, never mind me

[andrjohns]

Paul, I worked up an initial implementation on this branch. It's currently only for simple outcome likelihoods but let me know if the basic approach is consistent with how you would tackle it, or if you'd prefer a different method.

As an example, calling:

make_stancode(rating ~ treat + period + carry,
              data = inhaler, normalise = FALSE)

Gives the model block:

  // likelihood including all constants
  if (!prior_only) {
    target += normal_id_glm_lupdf(Y | Xc, Intercept, b, sigma);
  }
  // priors including all constants
  target += student_t_lpdf(Intercept | 3, 1, 2.5);
  target += student_t_lpdf(sigma | 3, 0, 2.5)
    - 1 * student_t_lccdf(0 | 3, 0, 2.5);
}

Which compiles and samples under the cmdstanr backend

[paul-buerkner]

Yes, this looks exactly like the approach I would have used!

[rok-cesnovar]

Nice! The problem is again this only works with stan2.25 and on.

[paul-buerkner]

Yes, so we should only merge once rstan is at 2.25 as well or make the stan code generation conditional, which I personally want to avoid.

[paul-buerkner]

Or just allow normalize = FALSE when cmdstanr with Stan 2.25+ is used, which would be fine and easy to check.

[andrjohns]

Great! Will work on updating and testing the rest of the likelihoods

[andrjohns]

Quick clarification for the priors, am I right in assuming that if normalise=TRUE gives:

  target += student_t_lpdf(Intercept | 3, 1, 2.5);
  target += student_t_lpdf(sigma | 3, 0, 2.5)
    - 1 * student_t_lccdf(0 | 3, 0, 2.5);

Then normalise = FALSE gives:

  target += student_t_lupdf(Intercept | 3, 1, 2.5);
  target += student_t_lupdf(sigma | 3, 0, 2.5);

In other words it no longer needs the lccdf adjustments for parameters with bounds, right?

[paul-buerkner]

Correct. Andrew Johnson <notifications@github.com> schrieb am Di., 10. Nov. 2020, 08:46:

…

Quick clarification for the priors, am I right in assuming that if normalise=TRUE gives: target += student_t_lpdf(Intercept | 3, 1, 2.5); target += student_t_lpdf(sigma | 3, 0, 2.5) - 1 * student_t_lccdf(0 | 3, 0, 2.5); Then normalise = FALSE gives: target += student_t_lupdf(Intercept | 3, 1, 2.5); target += student_t_lupdf(sigma | 3, 0, 2.5); In other words it no longer needs the lccdf adjustments for parameters with bounds, right? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1017 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADCW2AGSBIJIGEYGWRDJMU3SPDVWLANCNFSM4SSMAGPA> .

[andrjohns]

Thanks!

[andrjohns]

Paul - got this pretty close to a PR-ready state (on this branch). I've tested the models in the tests.model_new.R file, and all looks good. Do you have any other models that this should be tested across before I move to a PR?

[paul-buerkner]

Amazing, thank you! Testing all from model_new seems sufficient I think. Once you make a PR, I am happy to review it and perhaps make some edits.

[paul-buerkner]

I realize we can close this isse since your PR has beem merged already. Thank you again for working in it!