h-s relationship (closes #1488) #1498
Conversation
|
Perhaps you could have a look at this @ckyriazis? |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1498 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 121 121
Lines 4099 4154 +55
Branches 568 586 +18
=======================================
+ Hits 4093 4148 +55
Misses 3 3
Partials 3 3
☔ View full report in Codecov by Sentry. |
|
Uh oh, we need some CI love. However, we seem to be passing the tests. |
|
Closes #1499. (unless someone objects before this goes in) |
stdpopsim/dfe.py
Outdated
| Instead of a single dominance coefficient, a discretized relationship | ||
| between dominance and selection coefficient can be implemented: | ||
| if dominance_coeff_list is provided, then there is a mutations with selection | ||
| coefficient s with dominance_coeff_breaks[k-] <= s <= dominance_coeff_breaks[k] will |
|
Note: this will require a change in analysis2 here: https://github.com/popsim-consortium/analysis2/blob/eb16df28c44a1c3a04a1a5801172e806280f5c2f/workflows/ts2fs.py#L54 |
|
Hey @petrelharp want me to look at this PR? Or waiting on ADK? |
|
Sorry, a bit overwhelmed by github notifications - sure, that'd be great! |
|
I think this looks good! Runs good too. (Although I haven't worried about possible interactions with the rest of the code base, since I'm not too familiar with it) |
| // For each stdpopsim mutation type with an h-s relationship | ||
| // we have a sequence of assigned SLiM mutation types; | ||
| // the first is the one that gets produced by mutation, | ||
| // and the remainder are assigned by a mutation callback. |
There was a problem hiding this comment.
| // and the remainder are assigned by a mutation callback. | |
| // and the remainder are assigned by a mutation callback. | |
| // This sequence of mutation types is a subset of the combined | |
| // mutation types list, e.g., including neutral mutations, etc. |
There was a problem hiding this comment.
I don't actually understand what your clarifying sentence means - which is "this sequence"? What are you trying to clarify?
There was a problem hiding this comment.
"this sequence [of mutation types]" is "a sequence of assigned SLiM mutation types; the first is the one that gets produced by mutation, and the remainder are assigned by a mutation callback".
The sentence I offered explains that the sequence you wrote about is a subset of a greater sequence, if other types are present.
There was a problem hiding this comment.
I tried rewriting this to clarify a few times, but it didn't work - I don't think this is the place to explain that basic concept - it's a thing that we explain elsewhere, and this minor comment in the SLiM code isn't the place to go into a detailed description of which things get slim mutation types.
stdpopsim/slim_engine.py
Outdated
| if mt.dominance_coeff_list is None: | ||
| h_list = [mt.dominance_coeff] | ||
| else: | ||
| h_list = [0.5] # this value will not matter |
There was a problem hiding this comment.
is None or -1 or something better?
There was a problem hiding this comment.
None will cause an error. And, if it's like -1 or something weird then I can imagine people looking at that value and thinking "whoa something is wrong" even though it's not? (Since the value will appear in the SLiM script, just for mutations that are never kept around.)
There was a problem hiding this comment.
I'll add a more clarifying comment.
|
OK - I did most of your suggestions; see my comments for those I didn't. How's it look now, @chriscrsmith ? |
|
Huh, what happened? The tests were passing: https://github.com/popsim-consortium/stdpopsim/actions/runs/5602731956 |
Looks great! |
Here's an implementation of the "discretized h-s relationship" discussed at #1488. There is a small backwards incompatibility here; that is that the "slim_mutation_id" entry we record in the top-level metadata is now a list instead of a single value. I think this is okay because we'll be doing a new release (and so can mention this), and our storing of things in metadata is not documented yet anyhow.
Note in particular that with this, we will be decoupling the "mutation type" of SLiM from the "mutation type" of stdpopsim (ie a single stdpopsim mutation type may produce mutations of many SLiM mutation types). This is why it was perhaps a lack of foresight to model the DFE machinery after SLiM's mechanics rather than motivating biology.
To understand what's going on here, read "10.6 Varying the dominance coefficient among mutations" of the SLiM manual.