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

Ancient europe #941

Merged
merged 1 commit into from
May 13, 2022
Merged

Conversation

AliPearson
Copy link

Added demographic model of Ancient Europe.

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.

This looks good @AliPearson, thanks! I left a couple of comments/questions below.

Comment on lines 1580 to 1582
author="Allentoft et al.",
year="in progress",
doi="in progress",
Copy link
Member

Choose a reason for hiding this comment

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

Our defacto policy for model inclusion is that the model appears in a publication. Do you have an idea of when the manuscript is expected to appear? I think a preprint ought to be fine for inclusion here.

Copy link
Contributor

Choose a reason for hiding this comment

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

preprint definitely counts as "publication".

@petrelharp
Copy link
Contributor

Just checking if you know what to do next, @AliPearson? Either fix up the things mentioned above or discuss them, then push a new commit; hopefully that will fix the failing check (the docs build) as well. If all is good then, you'll then squash & rebsae and we'll merge! Let us know if you need more direction than that. Thanks, this looks great!

@AliPearson
Copy link
Author

AliPearson commented May 25, 2021 via email

@petrelharp
Copy link
Contributor

No worries - just checking in!

@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #941 (453543b) into main (cdc60bd) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

❗ Current head 453543b differs from pull request most recent head b577a30. Consider uploading reports for the commit b577a30 to get more accurate results

@@            Coverage Diff             @@
##             main     #941      +/-   ##
==========================================
- Coverage   99.56%   99.43%   -0.14%     
==========================================
  Files         103       84      -19     
  Lines        3248     2650     -598     
  Branches      415      296     -119     
==========================================
- Hits         3234     2635     -599     
- Misses          6        7       +1     
  Partials        8        8              
Impacted Files Coverage Δ
stdpopsim/catalog/HomSap/demographic_models.py 100.00% <100.00%> (ø)
stdpopsim/slim_engine.py 98.79% <0.00%> (-0.98%) ⬇️
stdpopsim/cli.py 98.16% <0.00%> (-0.44%) ⬇️
stdpopsim/__init__.py 94.73% <0.00%> (-0.27%) ⬇️
stdpopsim/utils.py 100.00% <0.00%> (ø)
stdpopsim/engines.py 100.00% <0.00%> (ø)
stdpopsim/genomes.py 100.00% <0.00%> (ø)
stdpopsim/citations.py 100.00% <0.00%> (ø)
stdpopsim/qc/HomSap.py 100.00% <0.00%> (ø)
stdpopsim/qc/__init__.py 100.00% <0.00%> (ø)
... and 32 more

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 cdc60bd...b577a30. Read the comment docs.

@AliPearson
Copy link
Author

Does this now look good? Ready to rebase?

@grahamgower
Copy link
Member

Yep, this looks like it's almost ready to go @AliPearson, I think we're just waiting for something citable (which can be a preprint)?

@andrewkern
Copy link
Member

hey @AliPearson -- any updates here? would love to merge this in

@petrelharp
Copy link
Contributor

Hi, @AliPearson - we're getting set to QC a bunch of things, so it'd be a good time to get this in - is there a good publication to cite?

@grahamgower
Copy link
Member

I think it's the recent Allentoft et al. manuscript: https://www.biorxiv.org/content/10.1101/2022.05.04.490594v2
Can you confirm @AliPearson?

@AliPearson
Copy link
Author

AliPearson commented May 11, 2022 via email

@petrelharp
Copy link
Contributor

Oh, awesome (and, congrats!). I thought it might be that.

@petrelharp
Copy link
Contributor

I'll add the citation and rebase this; then we should be good to go (do you remember otherwise, @AliPearson?)

@petrelharp
Copy link
Contributor

Ok - the rebase was a bit painful, but I got it sorted. I've updated your repository (since the one this PR points to), @AliPearson, in case you're not familiar with this, you'll have to do something like git checkout AncientEurope; git fetch; git reset --hard github/AncientEurope (where maybe it's origin instead of github?) - let me know if anything is amiss. And, I've got a backup of your original branch locally, in case we need to go back to it, but I'm pretty sure all your changes are in here.

@petrelharp
Copy link
Contributor

I've confirmed with @AliPearson that this is good to go, so I'll merge this and open the QC issue.

@petrelharp petrelharp merged commit 5c149f9 into popsim-consortium:main May 13, 2022
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.

None yet

4 participants