Skip to content
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

Add Gutunkunst OOA QC #496

Merged
merged 1 commit into from
Apr 30, 2020
Merged

Add Gutunkunst OOA QC #496

merged 1 commit into from
Apr 30, 2020

Conversation

apragsdale
Copy link
Member

@apragsdale apragsdale commented Apr 27, 2020

Implemented qc version of the 2009 OOA model (to close #492). I'm having issues getting the tests to catch if I made a mistake in implementation, and I think the development docs are out of date (specifically, item 5 under Demographic model review process, perhaps)? Do I need to add anything beyond qc_model = homo_sapiens_qc.GutenkunstOOA() to the TestGutenkunstThreePopOutOfAfrica class in test_homo_sapiens.py?

@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #496 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #496   +/-   ##
=======================================
  Coverage   99.52%   99.52%           
=======================================
  Files          16       16           
  Lines        1464     1464           
  Branches      186      186           
=======================================
  Hits         1457     1457           
  Misses          2        2           
  Partials        5        5           
Impacted Files Coverage Δ
stdpopsim/catalog/homo_sapiens.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c977918...8ba1b24. Read the comment docs.

Comment on lines 112 to 114
class TestGutenkunstThreePopOutOfAfrica(
unittest.TestCase, test_models.CatalogDemographicModelTestMixin):
model = species.get_demographic_model("OutOfAfrica_3G09")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue here as for the ZigZag model, regarding the need to instead inherit from QcdCatalogDemographicModelTestMixin.

@grahamgower
Copy link
Member

Thanks for taking on these missing QC models @apragsdale! Should be a simple change to get them doing the checks. And when #304 and #305 are resolved, no one else will get caught on changing which superclass to inherit from!

@apragsdale
Copy link
Member Author

Thanks, I had forgotten that needed to be changed. Coming back to implement models a few months later really does make it clear where the docs could be filled in :) I can do some updating of the QC process documentation after these are finished up.

@apragsdale
Copy link
Member Author

I did find a mistake in the OOA model currently in the catalog: in demographic events, migration was not turned off between the African and Bottleneck populations after they are merged, so that migration between those populations continues into the distant past.

I don't know if this will have affected any of the analyses in the paper, for example Figure 3 from the manuscript... But if so, we should probably discuss on the call tomorrow.

@grahamgower
Copy link
Member

That's a little worrying! @jeromekelleher can you comment on the likely behaviour of msprime here? Does a migration_matrix with non-zero entries have any effect for lineages already "joined" by a MassMigration(proportion=1) event?

@apragsdale
Copy link
Member Author

It definitely will, because migration will carry lineages back into the merged population, so you'll end up with longer expected Tmrcas for lineages still uncoalesced beyond that merger event. Since it's in the distant past, the effect will likely be small, but it will still be there.

@jeromekelleher
Copy link
Member

That's a little worrying! @jeromekelleher can you comment on the likely behaviour of msprime here? Does a migration_matrix with non-zero entries have any effect for lineages already "joined" by a MassMigration(proportion=1) event?

We don't touch the migration matrix when there's MassMigrations - decided events having side effects was confusing.

@apragsdale, does this mean that the OOA model that's in the msprime docs is wrong too? (Ouch!)

@apragsdale
Copy link
Member Author

@apragsdale, does this mean that the OOA model that's in the msprime docs is wrong too? (Ouch!)

Oh no!! It certainly looks like it.. That's surprising that hasn't been caught before, really. I've used that example in the msprime docs a bunch of times and never noticed it either.

@apragsdale
Copy link
Member Author

I think this (and the Zigzag model) are ready to merge. Let me know if you need me to rebase at all again for one or both.

@petrelharp
Copy link
Contributor

I have no idea why Travis is unhappy here - it is something to do with conda. Help, @grahamgower? @jradrion?

@grahamgower
Copy link
Member

It's an unrelated problem, and it looks like its been failing for two weeks now. Travis is running fine for linux on python 3.6. The failing builds are for osx --- maybe somethign changed at the travis end, and the .travis.yml file needs to have a language set explicitly for the osx case?

@jeromekelleher
Copy link
Member

Travis build should be fixed now if you rebase @apragsdale.

@apragsdale
Copy link
Member Author

This and #498 are now both rebased.

@petrelharp
Copy link
Contributor

Yay, thanks!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HomSap/OutOfAfrica_3G09 needs QCing
4 participants