fixed lgamma-based crashes (from neg_binomial)

[Bob Carpenter]

The problem was that an exception was being thrown
during the lgamma_vari constructor by Boost's lgamma
implementation. Daniel and I pulled the lgamma
calc out of the constructor, and now everything works
fine.

That negative binomial model doesn't crash any
more, but the model itself still doesn't work.

But I think that's just HMC, not a bug. We should
probably build some example negative binomial models
that work.

- Bob

[Matt Hoffman]

Is it still going off to +-inf or 0? If so, that's probably a bug, not
normal behavior for HMC. It smells to me like we've somehow got an
improper posterior. I can look into it, but not for a little while.

Matt

> --
>
>

[Daniel Lee]

On Tue, Aug 21, 2012 at 3:06 PM, Matt Hoffman <mdho...@cs.princeton.edu> wrote:
> Is it still going off to +-inf or 0? If so, that's probably a bug, not
> normal behavior for HMC. It smells to me like we've somehow got an
> improper posterior. I can look into it, but not for a little while.

Both. It used to crash when alpha got to 0 and beta got to +inf. I've
seen alpha and beta both go to +inf.

[Matt Hoffman]

Is it rejecting when that happens? If not, then there's a bug in
something being called by log_prob(), since p(log(alpha)=-inf) =
p(beta=inf) = 0.

> --
>
>

[Bob Carpenter]

To make matters easier, I added the model and data
to Git:

src/models/basic_estimators/negative_binomial.stan
/negative_binomial2.stan
/negative_binomial.Rdata

I also added the transformed version with a success
parameter instead of inverse scale.

There's also only a very small amount of data -- 9
observations with low counts between 0 and 10.

- Bob

[Andrew Gelman]

Improper posteriors should cause poor convergence but not crashes, infinite loops, etc!

> --
>

[Matt Hoffman]

We should probably have a check somewhere that tests if any parameter
goes (without rejection) to +-inf on its unconstrained scale, since
the only way that can happen is if the posterior is improper. Warning
users that they've specified an improper posterior is a valuable
service.

Matt

> --
>
>

[Bob Carpenter]

On 8/26/12 1:49 PM, Matt Hoffman wrote:
> We should probably have a check somewhere that tests if any parameter
> goes (without rejection) to +-inf on its unconstrained scale, since
> the only way that can happen is if the posterior is improper. Warning
> users that they've specified an improper posterior is a valuable
> service.

That's a very good idea and won't be hard.

It's now on the to-do list under "sampling".

I'm thinking we need to trap +inf, -inf in the
samplers, as they won't be actual samples because
of the Metropolis reject steps (though they could
still be in some cases).

I could just put it in the leapfrog() calculation.

I'm also assuming I should just stop when I hit
such a value. But then not if it's due to initialization,
because we want to allow some restarts in case we
got a bad init.

- Bob

[Marcus Brubaker]

I'd put the check in the parameter transforms with an error message
that suggests it may be due to an improperly specified model/prior.
The leapfrog should probably continue assuming it's not the first step
as it could (in theory) be a proper model.

Cheers,
Marcus

[Bob Carpenter]

On 8/26/12 6:24 PM, Marcus Brubaker wrote:
>> On 8/26/12 1:49 PM, Matt Hoffman wrote:
>>>
>>> We should probably have a check somewhere that tests if any parameter
>>> goes (without rejection) to +-inf on its unconstrained scale, since
>>> the only way that can happen is if the posterior is improper.

> I'd put the check in the parameter transforms with an error message
> that suggests it may be due to an improperly specified model/prior.
> The leapfrog should probably continue assuming it's not the first step
> as it could (in theory) be a proper model.

That would be really easy and should catch any
error in untransformed parameters being infinite.

It's below the level of our C++ API, though, so
it wouldn't catch the issues in models coded up
directly in C++. I suppose we could leave that to
the C++ users, as they'll be implementing the log
prob function that takes the unconstrained params
directly.

- Bob

[Matt Hoffman]

I'm not sure we want to die if we ever see a +-inf. There are two cases:
1. p(|θ_i|=∞) > 0
2. p(|θ_i|=∞) = 0.
Case 1 says we've got an improper posterior, and we should die in that
case. But case 2 just says that we've done a bad job of choosing a
step size, and so our leapfrog integrator has gone bananas. That's not
necessarily a huge deal, especially in the adaptation phase, so we
shouldn't do anything drastic. We could report it, though, if it's not
much of a pain.

Agree/disagree?

Matt

> --
>
>

[Michael Betancourt]

If you really want to be pedantic one also has to be careful in (1) because the Hamilton evolution breaks down on a non-compact manifold. I agree that (2) signals a bad transition but doesn't mean that the chain itself should be aborted.

> --
>
>