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

QC for OutOfAfrica_4J17 #1246

Merged
merged 4 commits into from
May 13, 2022

Conversation

jeffspence
Copy link
Contributor

QC for #733 (@grahamgower)

This is my first time doing QC -- so let me know if I'm doing anything wrong. I was able to (blind) replicate the model. The only issues I ran into were:

  1. The times in generations as currently implemented have all been cast to integers. e.g., in the notation of Table 4 here T_{CH-JP} is 9kya. The generation time is 29y so I originally implemented this as 9e3/29 but that caused the tests to fail. I could get the tests to pass if I changed this to int(9e3/29). The models are inferred by moments which assumes discrete (hence integer) generations so I guess that makes sense, but since they only report to the precision of 9kya, it's not clear whether it makes sense to convert to an integer or not.
  2. I initially used the newer msprime API to specify the population splits (add_population_split) but that caused one of the populations to become "inactive" which caused the tests to fail. I could get around this by switching to the old style of specifying the equivalent population splits as mass migrations, plus setting migration rates to zero. I guess this is maybe a personal preference kind of thing, so I'm happy to go either way.

otherwise my initial implementation matched the current implementation.

@jeffspence
Copy link
Contributor Author

jeffspence commented May 13, 2022

I should also say that my first commit on this branch is the model as I originally implemented it, and the second commit is the tweaks I mentioned above (switching generation times to integers, and switching to add_mass_migration)

@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #1246 (0294f0e) into main (4d01954) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1246   +/-   ##
=======================================
  Coverage   99.56%   99.57%           
=======================================
  Files         103      103           
  Lines        3248     3296   +48     
  Branches      415      415           
=======================================
+ Hits         3234     3282   +48     
  Misses          6        6           
  Partials        8        8           
Impacted Files Coverage Δ
stdpopsim/qc/HomSap.py 100.00% <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 4d01954...0294f0e. Read the comment docs.

ran black to fix some formatting nonsense
missed that `black` added some whitespace in GladsteinAshkSubstructure the previous commit that I have now removed.

I think that should do it!
@jeffspence jeffspence requested a review from grahamgower May 13, 2022 03:27
Copy link
Member

@grahamgower grahamgower left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jeffspence.

@grahamgower
Copy link
Member

QC for #733 (@grahamgower)

This is my first time doing QC -- so let me know if I'm doing anything wrong. I was able to (blind) replicate the model. The only issues I ran into were:

  1. The times in generations as currently implemented have all been cast to integers. e.g., in the notation of Table 4 here T_{CH-JP} is 9kya. The generation time is 29y so I originally implemented this as 9e3/29 but that caused the tests to fail. I could get the tests to pass if I changed this to int(9e3/29). The models are inferred by moments which assumes discrete (hence integer) generations so I guess that makes sense, but since they only report to the precision of 9kya, it's not clear whether it makes sense to convert to an integer or not.

Conversion to intergers is fine here. It makes sense in the context, and there will be no practical difference in simulation output as a result.

  1. I initially used the newer msprime API to specify the population splits (add_population_split) but that caused one of the populations to become "inactive" which caused the tests to fail. I could get around this by switching to the old style of specifying the equivalent population splits as mass migrations, plus setting migration rates to zero. I guess this is maybe a personal preference kind of thing, so I'm happy to go either way.

This can be dealt with by using the population_id_map parameter to DemographicModel. But its undocumented, so don't worry about it.

otherwise my initial implementation matched the current implementation.

Great! I'm going to merge this without squashing, because your first model version could be useful. Thanks!

@grahamgower grahamgower merged commit 329f702 into popsim-consortium:main May 13, 2022
@jeffspence jeffspence deleted the OutOfAfrica_4J17_qc branch May 13, 2022 16:42
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.

2 participants