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 bug in likelihood caused by #278. #288

Merged
merged 3 commits into from Nov 6, 2019
Merged

Conversation

peisenha
Copy link

@peisenha peisenha commented Nov 4, 2019

Current Behavior

The likelihood function is wrong as we currently evaluate the average of the log smoothed choice probabilities instead of the log of the average smoothed choice probabilities.

The bug was implemented in #278 and the lines are:

https://github.com/OpenSourceEconomics/respy/pull/278/files#diff-7d9b0e9ef0d4224d6a91d036420079b0L406-L410

Desired Behavior

  • We want the correct likelihood function.
  • logsumexp.pdf shows that the problem boils down to two logsumexp functions which is better than the softmax.

Todo

  • update docstrings
  • investigate the failure of our test battery to detect the change in likelihood evaluation

@tobiasraabe tobiasraabe changed the title bugfix for likelihood Fix bug in likelihood caused by #278. Nov 5, 2019
Copy link
Member

@tobiasraabe tobiasraabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with the changes, now. The speed is almost back to levels before #278 (+0.2s for 40k obs with kw_94_one). But, the function is more robust.

@tobiasraabe
Copy link
Member

@peisenha I cannot assign a review to you, but you should go over the changes again. Afterwards, I will hit approve.

Copy link
Author

@peisenha peisenha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it looks good and it is much faster now. What is our way forward for adding more sensitive regression tests? Are we going to create a new PR for that?

@tobiasraabe
Copy link
Member

At least the clipping PR in #286 will change the regression tests and then I will create different ones as proposed by Janos.

@tobiasraabe tobiasraabe self-requested a review November 6, 2019 10:06
Copy link
Member

@tobiasraabe tobiasraabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding the bug, @peisenha!

@tobiasraabe tobiasraabe merged commit e994590 into develop Nov 6, 2019
@tobiasraabe tobiasraabe deleted the peisenha_bugfix_likl branch November 6, 2019 10:56
@tobiasraabe tobiasraabe added this to the 2.0.0 milestone Nov 20, 2019
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.

None yet

2 participants