Skip to content

ts read and write (double cross over breakpoints included)#22

Open
LynxJinyangii wants to merge 13 commits into
HighlanderLab:tsfrom
LynxJinyangii:ts
Open

ts read and write (double cross over breakpoints included)#22
LynxJinyangii wants to merge 13 commits into
HighlanderLab:tsfrom
LynxJinyangii:ts

Conversation

@LynxJinyangii

Copy link
Copy Markdown

ts read, sampling SNPs, and write out after simulation (mainly in R)
double cross over breakpoints included (mainly in Cpp)

@codecov-commenter

Copy link
Copy Markdown

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered ☂️

@gregorgorjanc

Copy link
Copy Markdown
Member

@LynxJinyangii I had a quick look, but there are a lot of changes so it will take me time. Very quickly.

In R function documentation goes above the function when using Roxygen; no inside docstrings like in Python;) Please ensure all functions have at least some rudimentary docs so we know what the args are supposed to be (type, description, etc.). You will see in AlphaSimR that user facing functions have substantial docs, but also internal functions have some basic docs, so let's strive for that.

You don't need to mark code changes by "Jinyang edited/added/changed" - we have diff tool to track this.

It would be helpful if you communicate why we need another recHist (named recHistGen) instead of just modifying the existing one. We probably don't want to double the memory usage with effectively the same? information?

It would also be easier to review if you clearly set the intent behind the changes when you open a PR. Here is a good example gaynorr#276. Alternatively, a PR can just refer to an issue where the idea/implementation plan is laid out gaynorr#280.

Can you please briefly describe your changes here so we can understand the high-level approach before we get lost in the weeds of the code review?

@LynxJinyangii

Copy link
Copy Markdown
Author

@gregorgorjanc Thanks for the feedback. I'm currently adding ts things to runMacsTs, things at the R level (e.g. makeFoundersFromTs.R) might still change. I'll come back to documentation and comments once the core structure is more defined.

Why use RecHistGen?

  • Because I'm not sure how much of the downstream requires recHist, while recHist and recHistGen store slightly different information. Col 1: for both, original Hap (recHistGen: double xover between 2 markers included); Col2: start from where (recHist: index of SNP; recHistGen: positions in Morgan, which are for ts edges); (some examples in https://github.com/LynxJinyangii/AlphaSimR/blob/master/dev/notesRealBreakpoints.md, including a workflow from reading .tree file(s) to output extended one(s)).

Main changes:

  • All the changes in .cpp (and related scripts, e.g. Class-SimParam.R, crossing.R...) are basically for recHistGen
  • AlphaSimR2TsGen/AlphaSimR2Ts: get information from offspringPop and SP object (recHistGen included) to extend ts tables
  • makeFoundersFromTs (functions inside are changing as mentioned above): read genotypes (sampling and filtering included) and calculate genMap

@LynxJinyangii

Copy link
Copy Markdown
Author

To make the variants() -related functions work, I temporaily linked it to RcppTskit version on https://github.com/LynxJinyangii/RcppTskit/tree/add-multiple-functions-on-pr-131

@LynxJinyangii

Copy link
Copy Markdown
Author

Recent commits are about runMacTS (or a staged way for advanced users: 1. ancestry simulation; 2. mutation simulation; 3. inbred leaf expansion (optional); 4. convert TS to MapPop (optional; sampling/filtering + map construction). For the introduction of the structure, I think it would be good to start from dev/testMaCSTS4.Rmd; then testMaCSTS1.Rmd, testMaCSTS2.Rmd and testMaCSTS3.Rmd.

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