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 4pop HomSap ooa model from Jouganous et al 2017 #726

Merged
merged 1 commit into from
Jan 12, 2021
Merged

add 4pop HomSap ooa model from Jouganous et al 2017 #726

merged 1 commit into from
Jan 12, 2021

Conversation

rwaples
Copy link
Contributor

@rwaples rwaples commented Dec 30, 2020

I have attempted to implement the 4 population model presented in Jouganous et al 2017.

This is my first time contributing to stdpopsim.

@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #726 (bb01f61) into master (3b8a879) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #726   +/-   ##
=======================================
  Coverage   99.65%   99.66%           
=======================================
  Files          33       33           
  Lines        2337     2376   +39     
  Branches      295      295           
=======================================
+ Hits         2329     2368   +39     
  Misses          3        3           
  Partials        5        5           
Impacted Files Coverage Δ
stdpopsim/catalog/HomSap/__init__.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 3b8a879...bb01f61. Read the comment docs.

@jeromekelleher
Copy link
Member

Hi @rwaples, welcome to popsim! The model looks good to me, thanks very much for contributing. I think we should be able to merge this one pretty quickly, and then hopefully find someone to do the QC model. However, @grahamgower is more up-to-date on the protocols for adding new models and we should probably wait for his 👍 (I think he's back next week)

@rwaples
Copy link
Contributor Author

rwaples commented Jan 4, 2021

Thanks, sounds good!

In regards to the model, I wasn't quite sure how to deal with population sizes prior to when the populations should exist, so I set them to zero, but looking at other model I see that this isn't always done.

@jeromekelleher
Copy link
Member

In regards to the model, I wasn't quite sure how to deal with population sizes prior to when the populations should exist, so I set them to zero, but looking at other model I see that this isn't always done.

It's optional - once there's no lineages in a particular population, it doesn't really matter what its size is.

Copy link
Member

@apragsdale apragsdale left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @rwaples! This looks great. I left a few comments here. Like @jeromekelleher said, resetting parameters of populations with no lineages doesn't change the behavior, but might end up causing confusion during QC. Also, adding the timing buffer of 0.0001 to some events shouldn't be needed, since events are applied in the order (backwards in time) that they are specified (unless this causes some issue with the slim engine?).

Thanks again, and welcome to stdpopsim!!

stdpopsim/catalog/HomSap/__init__.py Outdated Show resolved Hide resolved
stdpopsim/catalog/HomSap/__init__.py Show resolved Hide resolved
stdpopsim/catalog/HomSap/__init__.py Outdated Show resolved Hide resolved
stdpopsim/catalog/HomSap/__init__.py Outdated Show resolved Hide resolved
stdpopsim/catalog/HomSap/__init__.py Show resolved Hide resolved
@rwaples
Copy link
Contributor Author

rwaples commented Jan 5, 2021

Thanks for the helpful comments, you folks have really made it easy to contribute here. As you both noticed I did have a bit of confusion in how to deal with populations before they really exist.

I have now removed the 0.0001 time buffers and also removed the demographic events that set pop sizes to zero when no lineages should exist in the population. I will resubmit the pull request soon.

I do plan to use this model (and others from stdpopsim) with the SLiM engine, via slim_script=True and with a bit of subsequent editing of the SLiM script file. I would be happy to help/test with that process if I can.

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.

Thanks @rwaples, this looks excellent! Would you be able to squash your commits into a single commit following the dev docs. I think this is ready for merging after that.

remove time buffers and demo events setting sizes to zero
@rwaples
Copy link
Contributor Author

rwaples commented Jan 11, 2021

Great. I tried to squash into a single commit

@grahamgower grahamgower merged commit 6774997 into popsim-consortium:master Jan 12, 2021
@grahamgower
Copy link
Member

Merged. Thanks again @rwaples! And please do let me know if you have any issues with modifying/using the script output by the SLiM engine (I trust you've seen the recap_and_rescale() function?).

@rwaples
Copy link
Contributor Author

rwaples commented Jan 12, 2021

Thanks!
I have seen recap_and_rescale(), but I didn't end up using that as I am pretty familiar with simulate() and didn't do any rescaling, nice to have though. I'm just using msprime.simulate(from_ts=).

The only real issue I have found with stdpopsim -> SLiM so far is that an msprime.CensusEvent() within the demographic events does not seem to have any impact when running with the SLiM engine. I have ended up manually editing the SLiM code to add "treeSeqRememberIndividuals". I can raise an issue about this if it would help., but I also understand if census events are not really meant to be supported.

FYI, this same paper has a related 5-pop model as well, so I will implement that once I get a chance.

@grahamgower
Copy link
Member

The only real issue I have found with stdpopsim -> SLiM so far is that an msprime.CensusEvent() within the demographic events does not seem to have any impact when running with the SLiM engine. I have ended up manually editing the SLiM code to add "treeSeqRememberIndividuals". I can raise an issue about this if it would help., but I also understand if census events are not really meant to be supported.

I don't see any reason why we couldn't support msprime.CensusEvent(), so feel free to open an issue for that.

FYI, this same paper has a related 5-pop model as well, so I will implement that once I get a chance.

👍

@rwaples
Copy link
Contributor Author

rwaples commented Apr 14, 2021

Quick question - There is an error in the description of this model (docs/parameter_tables/HomSap/OutOfAfrica_4J17.csv) (as noted in #757). I can fix this, but not sure how to proceed - should I do this with a new pull request?

@petrelharp
Copy link
Contributor

Yes - a PR is the way to go. 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.

5 participants