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

Potential issue in log likelihood calculation #7

Open
Gulfa opened this issue Oct 13, 2018 · 6 comments
Open

Potential issue in log likelihood calculation #7

Gulfa opened this issue Oct 13, 2018 · 6 comments
Labels

Comments

@Gulfa
Copy link

Gulfa commented Oct 13, 2018

I was trying to replicate the results of the ML estimate for R in this package without being successful. I have narrowed down the difference between my implementation and the one in this package to line 175 in get_R.R.

sum(stats::dpois(x[-1], lambda = R * x_lambdas, log = TRUE))

I might be misunderstand something, but I think that the length of x[-1] and x_lambdas are different in this line of code and the dpois function then recycles values from x[-1] which is shorter. From my understanding x_lambdas has one entry too many and the final element should be removed. I tried this in a fork and then the estimated value for R matched my python implementation. In the example in https://www.reconlearn.org/post/practical-ebola-reconstruction.html, this changes the estimated value of R from 1.286 to 1.336.

Happy to submit a PR from my fork if that would be helpful and I am not completely mistaken about this.

@annecori
Copy link
Collaborator

annecori commented May 29, 2019

@thibautjombart I agree with the above issue spotted by @Gulfa - maybe worth having a look?
One way of solving this I think is to not do the subsetting [-1] on line 170 but instead do it inside line 176 with sum(stats::dpois(x[-1], lambda = R * x_lambdas[-1], log = TRUE))

@thibautjombart
Copy link
Contributor

I think this is now fixed by the recent PR:
https://github.com/reconhub/earlyR/blob/master/R/get_R.R#L177-L206

@zkamvar zkamvar added the bug label May 29, 2019
@zkamvar
Copy link
Member

zkamvar commented May 29, 2019

I don't think this is exactly fixed. The first lambda is now NA

@thibautjombart
Copy link
Contributor

@annecori will confirm, but I think the first lambda is NA: the force of infection prior to the first case is unknown, and the first non-negative incidence actually does not enter the likelihood calculation.

@annecori
Copy link
Collaborator

yes that's correct

@zkamvar
Copy link
Member

zkamvar commented May 30, 2019

@Gulfa, can you check the most recent github version of earlyR to check if it's correct?

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

No branches or pull requests

4 participants