-
Notifications
You must be signed in to change notification settings - Fork 87
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
QC for AncientEurope_4A21 #1256
QC for AncientEurope_4A21 #1256
Conversation
26b8cc3
to
13e31a5
Compare
Nice work @mufernando! The model comparison is failing with: |
Ah, thanks! We'd got this figured out, and our plan was for @awohns to re-write the model with the new msprime API, but perhaps this is an easier solution? (And, once they agreed, we were going to switch the implementations, putting the yaml one in main and the msprime one in qc.) |
thanks for the reply @grahamgower. I reimplemented the model using demes and added it to a separate YAML file for now under |
4baa0f9
to
e6885e8
Compare
I'm having two issues here:
|
b5e866d
to
fe4fac3
Compare
Perhaps @apragsdale can provide some input? |
Before diving into the weeds of doing the model comparisons here @mufernando, what's the reasoning for bringing Demes in here? I don't think we've used Demes as input anywhere else (unless I missed it) and there will be annoying quirks here in the different expressions of the same models between old-skool msprime, msprime 1.0 Demography API and Demes. It would be better to keep things uniform until we can do the catalog update, wholesale. I agree it's a pain to have to use the old APIs when the new tools are so much better, though. |
That's a good question. The model is fairly complex and I wanted to make
sure it was right by iteratively checking the model with demesdraw as I
built it. I can reimplement it using the msprime API, but this is a good
hands-on opportunity to test the demes integration out?
…On Thu, May 19, 2022, 12:30 PM Jerome Kelleher ***@***.***> wrote:
Before diving into the weeds of doing the model comparisons here
@mufernando <https://github.com/mufernando>, what's the reasoning for
bringing Demes in here? I don't think we've used Demes as input anywhere
else (unless I missed it) and there will be annoying quirks here in the
different expressions of the same models between old-skool msprime, msprime
1.0 Demography API and Demes. It would be better to keep things uniform
until we can do the catalog update, wholesale.
I agree it's a pain to have to use the old APIs when the new tools are so
much better, though.
—
Reply to this email directly, view it on GitHub
<#1256 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACL5HFBRSKILDAGLCJ43HZ3VK2JFJANCNFSM5WCQSY6Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I would vote for doing one thing at a time, as there are enough moving parts already. I'm not really doing much here though at the moment, so I'd give other votes more weight! |
I don't have any good suggestion regarding sampling times. But just sticking to the msprime API for now is probably the path of least resistance. You can always call |
We should keep the nicely crafted Demes model around somewhere though, as we'll definitely want to use this when we do the technology transition for the catalog. Maybe we open an issue with it and keep the model in there? Or just leave the model in the file, commented out? |
I reimplemented the model now using |
also, it's not clear to me what to do with the Bronze age population. It has two sampling times but for now we only implemented one. |
Thanks for the heavy lifting here! I say just put in one sampling time (the more recent one?). |
Codecov Report
@@ Coverage Diff @@
## main #1256 +/- ##
=======================================
Coverage 99.65% 99.66%
=======================================
Files 111 111
Lines 3523 3558 +35
Branches 438 438
=======================================
+ Hits 3511 3546 +35
Misses 8 8
Partials 4 4
Continue to review full report at Codecov.
|
Minor changes to fix two failing tests:
Look ok @mufernando? |
c37acd1
to
df34cc1
Compare
I think the last thing we need before we merge this is to figure out where Alice Pearson got the |
I chose the growth rate pretty arbitrarily. I wanted to make the population size grow exponentially from the bronze age to present-day, resulting in a large enough population size to prevent coalescences in the first 100 generations or so. Not sure how realistic this is and i have since changed it to be constant from the bronze age, but that is what is in the preprint. |
Oh, good - so the number is actually in the preprint? We didn't find it, but there is a lot in there. |
If it's not in the preprint (and I just checked again?), we should just put in a comment saying it's a personal communication. |
No problem! I've added a note about that, so I think we're ready to merge in! Thanks, all! |
added note about growth rate
d6525c2
to
7107232
Compare
@awohns and I QC'ed the AncientEurope_4A21.