Cleanup code in neg_binomial_2_lpmf and neg_binomial_2_log_lpmf

[rok-cesnovar]

Description

There is some potentially dangerous code in neg_binomial_2_lpmf and neg_binomial_2_log_lpmf that I think should be made more robust. There is also possible there is a bug.

Example

This example is from neg_binomial_2_lpmf but neg_binomial_2_log_lpmf has a similar example.

In line 47 we have

size_t size = max_size(n, mu, phi);

A bit later down we have

  for (size_t i = 0, size = length(mu); i < size; ++i) {
    mu__[i] = value_of(mu_vec[i]);
  }

  for (size_t i = 0, size = length(phi); i < size; ++i) {
    phi__[i] = value_of(phi_vec[i]);
  }

  for (size_t i = 0, size = length(phi); i < size; ++i) {
    log_phi[i] = log(phi__[i]);
  }

So we basically overwrite the size we calculated in line 47 which is never used. And we then have

for (size_t i = 0; i < size; i++) {
    if (include_summand<propto>::value) {
      logp -= lgamma(n_vec[i] + 1.0);
    }
    if (include_summand<propto, T_precision>::value) {
      logp += multiply_log(phi__[i], phi__[i]) - lgamma(phi__[i]);
    }
  // much more code

Can someone with the knowledge of this codebase double-check. It does seem at least a bit fishy.

Current Version:

v3.0.0

[mcol]

See #1496 where some of these problems have already been noticed.

[syclik]

Thanks, all. That’s bad.

…

On Fri, Dec 20, 2019 at 2:59 PM Marco Colombo ***@***.***> wrote: See #1496 <#1496> where some of these problems have already been noticed. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1531?email_source=notifications&email_token=AADH6F2GO3XCK6RNV33U2KDQZUPZXA5CNFSM4J6BHMZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHOAV6I#issuecomment-568068857>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADH6F5IO3DMGR6EX2CTC5DQZUPZXANCNFSM4J6BHMZQ> .

[rok-cesnovar]

@mcol thanks, I went to check the PR addressing that issue and I didnt see this being addressed. Not sure this contributes to the problem @martinmodrak is investigating, which I think he also fixed with the PR. It doesnt help probably.

[martinmodrak]

@rok-cesnovar I agree that's fishy. I also think fixing it for neg_binomial_2_lpmf along #1496 in the PR #1497 makes sense, as otherwise there would be just merge conflicts. I am currently not touching neg_binomial_2_log_lpmf, so this would need to be fixed separately, possibly when handling #1495, but I don't think I'll be able to work on #1495 anytime soon, so might be worth fixing separately.

[mcol]

These are actually benign, as in doing for (size_t i = 0, size = length(mu); i < size; ++i) the variable size is actually being declared in the inner scope of the for loop, so it doesn't affect the one declare outside of it. I only realised this was the behaviour through tests, it's definitely very confusing!