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 new_kappa allocation #53

Merged
merged 1 commit into from
Nov 15, 2018
Merged

Conversation

alxsrobert
Copy link
Contributor

With the previous script, the allocation of new_kappa using param["kappa"] was interfering with the pointer definition. It was then causing errors in the computation of the new and old likelihoods (new_likelihood then corresponded to the old_likelihood).
In outbreaker_find_imports: there was a mistake in the position of the parentheses for the calculation of n_measures.

With the previous script, the allocation of new_kappa using param["kappa"] was interfering with the pointer definition. It was then causing errors in the computation of the new and old likelihoods (new_likelihood then corresponded to the old_likelihood).
In outbreaker_find_imports: there was a mistake in the position of the parentheses for the calculation of n_measures.
@finlaycampbell
Copy link
Collaborator

Cheers for that @alxsrobert, sorry for slow reply I was away at a conference. Funnily enough I found that bug a couple weeks ago too, turns out I'd only pushed the fix to my own fork and not the main fork! I will be doing a merge of your bug fixes and some of my own additions over the next couple days. Turns out there was actually a theoretical bug in the genetic likelihood itself, which unfortunately may have affected kappa estimates. I will post to the README describing the changes in the likelihood when I merge my fork. Thanks again!

@thibautjombart
Copy link
Collaborator

I'll leave this one with you both - thanks! :) Probably stating the obvious, but best make sure that:

  • all previous tests are green
  • add a new test for this problem specifically
  • add @alxsrobert to the list of contributors in DESCRIPTION

Copy link
Collaborator

@finlaycampbell finlaycampbell left a comment

Choose a reason for hiding this comment

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

Thanks again!

@finlaycampbell finlaycampbell merged commit 0228798 into reconhub:master Nov 15, 2018
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.

3 participants