WIP: selection for slim engine. #462
Conversation
Codecov Report
@@ Coverage Diff @@
## master #462 +/- ##
==========================================
+ Coverage 99.65% 99.67% +0.02%
==========================================
Files 29 31 +2
Lines 2000 2172 +172
Branches 207 253 +46
==========================================
+ Hits 1993 2165 +172
Misses 3 3
Partials 4 4
Continue to review full report at Codecov.
|
|
Cool! I'm not sure about the |
|
I was only thinking there needed to be another namespace for stuff that might not be supported in all engines (or for all demographic models?). I don't mind much what it's called. I wasn't sure how much of this would be selection-specific, and how much would be slim-specific. Possibly we'll end up with one selection module that works for msprime and slim, but maybe not. I hope this concrete example provides some useful information about what sort of interface could work on the slim side. It would be really nice if the interface(s) could be engine-agnostic and also "extension feature"-agnostic. To that end, it might be worth simultaneously considering one or more non-selection features. |
|
Sounds good. How about we put in a comment in the |
grahamgower
force-pushed
the
selection
branch
3 times, most recently
from
fd52965 to
f8c2920
Compare
grahamgower
force-pushed
the
selection
branch
2 times, most recently
from
87b5904 to
258e859
Compare
|
Could you remind us of how to emit the SLiM script to have a look at? |
| ts = engine.simulate( | ||
| model, contig, samples, | ||
| seed=seed, | ||
| mutation_types=mutation_types, | ||
| slim_scaling_factor=10, | ||
| slim_burn_in=10, | ||
| # Set slim_script=True to print the script instead of running it. | ||
| # slim_script=True, |
There was a problem hiding this comment.
@petrelharp: pull down the pr branch, then edit this line of docs/selection_example.py, and run it to emit the slim script to stdout.
| frequency conditioning. | ||
|
|
||
| FIXME: Drawing multiple mutations using the same mutation type causes | ||
| erroneous allele frequency calculation in the SLiM code! |
There was a problem hiding this comment.
what do you mean here more specifically, @grahamgower?
There was a problem hiding this comment.
The allele frequency calculation is a bit fragile. I didn't think too hard about it when I originally wrote it, because my use case was limited to drawing only one mutation. What do you think, should we change this af() function, or disallow multiple drawn mutations of the same mutation type?
// Return the allele frequency of a drawn mutation in the specified population.
// Assumes there's only one mutation of the given type.
function (float$)af(object$ mut_type, object$ pop) {
mut = sim.mutationsOfType(mut_type);
if (length(mut) == 0) {
return 0.0;
}
return sim.mutationFrequencies(pop, mut);
}
There was a problem hiding this comment.
This looks great to me. Really amazing work, @grahamgower! What do we need to do to push this over the finish line (apart from more careful documentation)?
|
Hey @mufernando, thanks for taking another look. I think the most important thing is to make sure the python API (all the extended events stuff) is appropriate. We want something that is extensible --- so that we can add new features in the future while keeping backwards compatibility. But if there's nothing obvious that could be improved here, maybe we just get it merged and worry about that if/when the time comes? (I see you made a comment and then deleted it: I think a simple additional feature could be to accept a starting allele frequency for a drawn mutation) |
|
One additional thought @mufernando: what if folks want to define a DFE with a custom |
grahamgower
force-pushed
the
selection
branch
from
8354ea3 to
7dbd835
Compare
|
Rebased. @jeromekelleher, would you be able to take a look at the python/API side of things here? |
There was a problem hiding this comment.
I think it looks great @grahamgower. I'm not sure about the value of the "ExtendedEvent", but we're being fairly explicit that this is an extension and stuff might change. So, I say we merge this much and start using and building on it!
|
Ok, cool. I'll squash this down, push the "Merge" button, and open some follow up issues. |
This supports drawing a mutation, changing it's fitness effects at a chosen time, and conditioning the simulation on the mutation having a given frequency in a given population at a given time. We also support simple DFEs via SLiM's initializeMutationType.
grahamgower
force-pushed
the
selection
branch
from
96a7965 to
380ab4f
Compare
Not for merging until at least after the next stdpopsim release. Feel free to ignore this for now. (Unless you're feeling bored, in which case feedback is welcome!)