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 sus scrofa #1312 #1672

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

AprilYUZhang
Copy link

No description provided.

@gregorgorjanc
Copy link
Contributor

I will work with @AprilYUZhang on revising this PR (for issue #1312) and the follow-up QC.

_ZhangEtAl = stdpopsim.Citation(
doi="https://doi.org/10.1016/j.gpb.2022.02.001",
year=2022,
author="Mingpeng Zhang et al.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to author="Zhang et al.",

_JohnssonEtAl = stdpopsim.Citation(
doi="https://doi.org/10.1186/s12711-021-00643-0",
year=2021,
author="Martin Johnsson et al.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to author="Johnsson et al.",

"16": 1.1407464686635653e-08,
"17": 1.3779949516098884e-08,
"18": 1.3679923888648167e-08,
"X": -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should provide a value for X chromosome too. I appreciate that Johnsson et al. (2021) does not list X-chromosome recombination rate. Hence, I suggest that you calculate average recombination rate across other chromosomes (exclude Y and MT) and replace this for the X-chromosome rec rate value. Make this calculation clear in the code here.

# citation is optional and can be deleted if not needed.]
citations=[
stdpopsim.Citation(
author="", year=-1, doi="", reasons={stdpopsim.CiteReason.ASSEMBLY}
Copy link
Contributor

Choose a reason for hiding this comment

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

@AprilYUZhang provide reference for the pig genome assembly that we use here

common_name="Pig",
genome=_genome,
ploidy=2,
# The age at first pregnancy varies in the wild from about 10 to 20 months,
Copy link
Contributor

Choose a reason for hiding this comment

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

@AprilYUZhang cite the paper here in the comments and point to page/figure/table for there estimates

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I have no idea; this sentence is from the explanation of generation time. Should I write "the details see in _ZhangEtAl"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Point to where in the paper is this detail, say page

author="Martin Johnsson et al.",
reasons={stdpopsim.CiteReason.REC_RATE},
)
# [The following are notes for implementers and should be deleted
Copy link
Contributor

Choose a reason for hiding this comment

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

@AprilYUZhang cite the paper here in the comments and point to page/figure/table for there estimates

Copy link
Contributor

@gregorgorjanc gregorgorjanc left a comment

Choose a reason for hiding this comment

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

@AprilYUZhang I left some comments for you to address. I will then have another look. Thanks for this addition to the stdpopsim catalogue!

"MT": 0,
}

# the de novo mutation rate is inferred by Mingpeng Zhang et al 2022,
Copy link
Contributor

Choose a reason for hiding this comment

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

@AprilYUZhang point to page/figure/table for there estimates from Zhang et al

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (237d101) to head (65c07e7).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1672   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files         136      139    +3     
  Lines        4690     4718   +28     
  Branches      470      470           
=======================================
+ Hits         4683     4711   +28     
  Misses          3        3           
  Partials        4        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gregorgorjanc
Copy link
Contributor

I advised @AprilYUZhang offline on this implementation, which is now very complete, and I volunteer to QC it.

I have a question though about recombination rate averaging. The information on the recombination rate for this species comes from a study with an extensive pedigree (so we deem it's very good) and the study reported the recombination rate per Mb of each autosome. To get a value for the X chromosome, @AprilYUZhang has:

  1. summed these recombination rates per Mb for each chromosome and divided by the chromosome length, to get recombination rate per basepair for each autosome
  2. calculated weighted average to get average recombination rate per basepair across the genome
  3. used the value from 2) for the X chromosome.

I think this makes sense, but would like to hear from others here what you have done in such cases. I think that weighted average to represent the recombination rate per basepair for the genome is the right thing to do (average of ratios ...). But is this the right value to use for the X chromosome (in the absence of other information) or should one use the simple (unweighted) average. It's a minor detail, but I thought I would ask here.

Otherwise, I think this looks very good @AprilYUZhang and could soon be merged in by the maintainers. You will have to squash the commits before this happens though!

@AprilYUZhang is also looking at adding the recombination map and demographic models later.

@petrelharp
Copy link
Contributor

Weighted average is consistent with what we do elsewhere - sounds good!

# which is roughly equivalent to the average age of
# the first pregnancy in sows and the beginning of rut in boars
# plus the pregnant gestation period of sows."
generation_time=3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm - generation time isn't the average age of first pregnancy of an individual, it's the average age of a parent at time of birth of their child, i.e., the thing you'd get by picking a random individual and asking how old their parent was when they were born. For instance, human generation time is 25-30 years (estimates vary).

Copy link
Contributor

Choose a reason for hiding this comment

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

@petrelharp your are totally right, but this is verbatim text from the paper that we are following, so I guess we stick to their 3 years, which is an OK value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Um, I guess? I mean, according to the stated rationale, that explanation is an underestimate, probably by a good factor, unless most pigs die after one litter.

This paper used 5 years, which the paper you're citing cites several other papers as using? Hm, that's "based on a studbook"; I wonder what the method is?

However, googling suggests that a mean life span is like 4-8 years; so 3 can't be too far off (maybe just a factor of 2). If you do want to use this one, then make sure you document that it is not the actual generation time.

@petrelharp
Copy link
Contributor

Looks good! Just a thing about the generation time.

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.

3 participants