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

fix log_likelihood function #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tokoharu
Copy link

There is wrong statement in the function log_likelihood.

Specifically, there is an error in the calculation of the normal distribution, which I would like to correct with this request.

@pfmonville
Copy link

Hi,
I wonder if the integer division is really what’s wanted here.
In method log_likelihood line 26 with this part -(rd**2)/2/sigma2[i] since rd is an integer when I go through your test case I obtain 6.328759366440833e-15 instead of 1.2091379396665127e-07 the first time it hits line 26.
(at first rd is -1 so:
(-1)²=1 -> 1/2=0 instead of 0.5)
Is this the desired behaviour ?
If this is the calculation of the normal distribution I think it should be a floating point division.

With floating point division I got a different result for the first day, then it converges.
yours -> mine
-102.65265035957913 -> -69.65648196168772
-1.9397850625546684 -> -1.9397850625546684
-2.1269280110429727 -> -2.1269280110429727
-0.6931471805599453 -> -0.6931471805599453

@tokoharu
Copy link
Author

@pfmonville
Thank you for pointing it out!!
It's not desired behaviour.
I'll fix it later, so please wait.

@tokoharu
Copy link
Author

@pfmonville
I confirmed that, in the latest commit, result for the test is same as your result.
I'm not good at Ruby, so, thank you again for pointing out my mistake.

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

Successfully merging this pull request may close these issues.

2 participants