-
Notifications
You must be signed in to change notification settings - Fork 92
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
Polish dog/wolf demography from #905 #1632
Polish dog/wolf demography from #905 #1632
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1632 +/- ##
=======================================
Coverage 99.84% 99.84%
=======================================
Files 130 132 +2
Lines 4416 4496 +80
Branches 608 608
=======================================
+ Hits 4409 4489 +80
Misses 3 3
Partials 4 4 ☔ View full report in Codecov by Sentry. |
@agladstein could you have a look if you agree with my changes? I think they are straightforward. I have also been doing QC offline against the publication. @nspope or @petrelharp could you review and hopefully merge - I have QC code ready and will share that in a separate PR. |
Hm - we're missing something to get this model hit by tests... |
LGTM. I notice a few difference between your version and my last version - but, I think you are correct in your updates. (Looks like I had the wolf pop sizes swapped). |
I think I found the reason! Let's see what GitHub actions say. |
@agladstein I get this error now that tests run
I suspect this is because of how we removed Boxer - this line
became
Do you have a suggestion on how to handle this? Perhaps we remove |
Yes - that make sense. I was considering pointing that out, but I thought based on the msprime docs that it would still work. If that doesn't work, I can try playing with it locally (but, I'm on a new computer and don't have my environments properly set up yet). |
To address this issue, I have now used the trunk model example suggestion from https://tskit.dev/msprime/docs/stable/demography.html#trunk-population-models This now works on my end
@agladstein please check if these changes make sense. |
Hmm, I see that tests on GitHub fail though!? See https://github.com/popsim-consortium/stdpopsim/actions/runs/12556941637/job/35008826399?pr=1632
Any idea what is causing this error? When I install locally from my git repo fork, I don't get this error as shown above at #1632 (comment) |
Splitting two pops into one is better practice than having a 'trunk' lineage in this case, since AFAIK neither BOX nor BSJ are a natural stand-in for ancestral dog. So, that's not the issue. |
@petrelharp we can’t include BOX because the publication didn’t estimate BOX parameters:( So, I have to use this trunk trick, I think. Happy to do something else, suggestions welcome. |
Not sure why you don't get the error locally, but GitHub fails. I was hoping to play with it locally myself, but I'll be out for next 2 weeks. |
Ah, right, sorry - now I've read the information above. And, after some digging, I know what's going on. It's a bit of a mess. Here's the reason for the error: if I insert
Digging in more, the problem is the samples from (gee, that was annoying to track down - I kept getting distracted by thinking it was to do with getting samples from ancestral pops) |
Great, now the docs build is failing with this (reproduced locally):
Here's the full log: I've not got the bandwidth to debug this at the moment; has anyone else? |
Never mind - it's because the parameter tables are (unquoted) CSV but some of these parameters have commas in their names:
should be
|
I'm going to merge this! Could someone open the QC issue, please? |
Thanks @petrelharp! |
I have taken the code from the draft PR at #905 and:
I volunteer to QC this model to learn the process.