ts read and write (double cross over breakpoints included)

[LynxJinyangii]

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

[codecov-commenter]

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]

@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]

@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]

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]

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.

[LynxJinyangii]

See https://github.com/LynxJinyangii/AlphaSimR/blob/ts/dev/testTsForwardRecorder.Rmd for commit add TSFowardRecoder and fix rec map for forward simulation

[LynxJinyangii]

See https://github.com/LynxJinyangii/AlphaSimR/blob/ts/dev/testTsForwardRecorderVcfInput.Rmd for a2d3890

[gregorgorjanc]

@LynxJinyangii as mentioned it’s MaCS and we have runMacs(), so please follow that across the code base.

[gregorgorjanc]

Please add small examples to all functions so we can test them as a user what they do and also showing the basic use cases

[gregorgorjanc]

Please document all functions or we loose understanding of the code

[gregorgorjanc]

@LynxJinyangii Looks like all of these are noRd, so there won’t be many pages or what happens?

[gregorgorjanc]

@LynxJinyangii will we need this now that you have added/sorted TS recording in C++?

[gregorgorjanc]

@LynxJinyangii I had a quick look and I am getting lost in the code - you have done a lot and likely used LLMs a lot too (which is ok), but we now have lots of code change across several files and looks like 20+ functions and I am not clear what is needed and what not. I appreciate you have done a lot of testing and several iterations, but it seems that we now have all of this mushed up together into this PR, so you are pushing onto me now to tease all this out, figure what you did, review it, test it, which will not work. For me to make a sense I need:

1. a clear overview of the added functionality - at the moment I don’t know where to start (I will use LLMs for this, but still, we need a human-written overview)

2. only the essential changes in the code without the old attempts that we abandoned (R recHist to ts conversion vs C++ ts recording) - you can see in the diffs all the code changes you have done

3. the functions should be documented, including examples, and unit tested

4. vignette(s) should be in the vignette or article folder (I think you have that for MaCS side, but not for tree sequence recording?)

Will you be able to address this?