-
Notifications
You must be signed in to change notification settings - Fork 1
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
Kingman prototype #1
base: master
Are you sure you want to change the base?
Conversation
/* In case of a recombination event in the Kingman phase | ||
* we require exactly one recombination point. | ||
*/ | ||
if(arg_flag == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style nit: there should be a space between the if and the bracket.
This is superb, thanks @JereKoskela! I think this is a great framework, and we'll be able to build the remaining functionality pretty easily on top of this. There are some minor changes I'd like to see:
|
I think I caught all the style discrepancies and removed the binary files. I also split up the z[] and r inits like we discussed. Self->num_parents still gets modified as before. Let me know if you spot anything else. |
[Dolphin] | ||
Timestamp=2015,6,24,17,30,18 | ||
Version=3 | ||
ViewMode=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be removed as well (KDE eh?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to remove 'main' from the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both done
@@ -270,6 +281,10 @@ run_sim(const char *config_file) | |||
} | |||
not_done = ret != 0; | |||
} | |||
if(self->simulate_kingman == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space needed here - if (..)
recomb_rates[j] = recomb_rates[j - 1] + recomb_rates[j] / total_rate; | ||
} | ||
coin = gsl_rng_uniform(self->rng); | ||
break_point = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation.
OK, I've had a more detailed look through this now, and it's clearer what's going on. You've done a great job integrating in with the existing code, but I wonder if perhaps you've gone a little too far in this. I think it would simplify things a lot if we made top level functions to perform the two types of event we want in the Kingman phase. It would be a lot easier to follow what's happening then, and we wouldn't need to trace through a bunch of functions using flags to determine what code path to take. Does this make sense? |
*/ | ||
int i, break_point = -2; | ||
double *recomb_rates = NULL; | ||
recomb_rates = xmalloc(gap_no * sizeof(double)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check if this is null here:
if (recomb_rates == NULL) {
ret = ERR_NO_MEMORY;
goto out;
}
Two new functions:
I've tried some trial simulations with small numbers of lineages and both few and many loci, and everything looks good. Valgrind also reports no memory leaks. But I haven't done extensive testing, which would probably be easier with a Python wrapper.