Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw error earlier for NaN logDensity #6

Open
jewellsean opened this issue May 15, 2014 · 5 comments
Open

Throw error earlier for NaN logDensity #6

jewellsean opened this issue May 15, 2014 · 5 comments

Comments

@jewellsean
Copy link
Collaborator

It would be beneficial, for future debugging purposes, if RealVariablePeskunTypeMove threw an error if any of the connected factors returns NaN. The error should include which factor returned NaN. My only hesitation is whether the check is worth the computational overhead.

@neilspencer
Copy link
Collaborator

If you're worried about the computational overhead, you might want to consider using an assertion to do the check. Then you can just turn them on or off when it is convenient.

@jewellsean
Copy link
Collaborator Author

Thanks, Neil. I will take a closer look into assertions.

After digging into this problem for the last while, I found some surprising behavior within the crpAssignmentLogProbabilitiy function. This function calculates the probability of a particular partition of Z into clusters based on parameters \alpha_0 and d. This is facilitated by using Apache's LogGamma function.

Apache's implementation of LogGamma(x) returns NaN for x< 0. (For reference, MatLab returns -\infty, Mathematica returns a complex number, and R returns the real part of the complex number.)

Previously, returning NaN was not a problem in polya (per se, but every subsequent proposal is rejected for negative initial \alpha_0). For the same reasons, it is not a problem for MHProposal moves, but creates an error for moves like RealVariablePeskunTypeMove, which cannot compare NaN when forming forwardLogNorm. For now, I am simply going to use MHProposals, however suggestions to deal with Apache's implementation are welcome.

@alexandrebouchard
Copy link
Collaborator

Interesting!

As for behavior with NaN... I agree that the user should be informed about NaNs. Stopping the sampler with an exception might be frustrating in some instances as the behavior of not accepting moves that lead to NaN is not unreasonable. But definitely at least some warnings in case this leads to the discovery of an underlying bug.

@jewellsean
Copy link
Collaborator Author

Thanks, Alex. I agree that perhaps throwing errors during sampling could be annoying. The real issue, I think, is that PeskunTypeMove cannot handle NaN so the sampling procedure crashes. This shouldn't be too difficult to fix.

@jewellsean
Copy link
Collaborator Author

My initial testing for this issue has revealed that this issue most commonly occurs when two components have loglikelihoods of -Inf => their sum -Inf + -Inf is NaN. There is one explicit loop in RealVariablePeskunTypeMove where this occurs, but could also occur in user's custom code.

The easiest fix is to simply return -Inf for the first component that evaluates as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants