Skip to content

Adding variants iterator and other functions#132

Open
LynxJinyangii wants to merge 5 commits intoHighlanderLab:mainfrom
LynxJinyangii:add-multiple-functions-on-pr-131
Open

Adding variants iterator and other functions#132
LynxJinyangii wants to merge 5 commits intoHighlanderLab:mainfrom
LynxJinyangii:add-multiple-functions-on-pr-131

Conversation

@LynxJinyangii
Copy link
Copy Markdown
Collaborator

No description provided.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 98.08307% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
RcppTskit/R/RcppTskit.R 94.91% 3 Missing ⚠️
RcppTskit/src/RcppTskit.cpp 98.18% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@gregorgorjanc gregorgorjanc changed the title Add multiple functions on pr 131 Adding variants iterator and other functions Apr 10, 2026
@gregorgorjanc gregorgorjanc force-pushed the add-multiple-functions-on-pr-131 branch from c443f64 to c309e3a Compare April 10, 2026 10:24
@gregorgorjanc
Copy link
Copy Markdown
Member

I have rebased this to the most recent main branch that contains previous PRs.

@gregorgorjanc
Copy link
Copy Markdown
Member

gregorgorjanc commented Apr 11, 2026

There is quite a bit to review in this PR, so best to make a list of new things! @LynxJinyangii I would appreciate such communication so it's easier to follow your work and thoughts etc. Best to err on the slight "over communication" that "no communication";) Even just copying NEWS.md entries or linking to them in the PR message would be fab.

From what I gather, this is the list of additions we should review:

  • rtsk_table_collection_sort and TableCollection$sort related to Do we have to add TableCollection$sort() method? #99
  • rtsk_treeseq_get_samples and TreeSeqeunce$samples (do we need samples for TableCollection then too? - best to discuss; edit - there is no such method for TableCollection)
  • rtsk_node_table_get_row and TableCollection$node_table_get_row (do we need node_table_get_row for TreeSequence too? - best to discuss)
  • rtsk_const_tsk_no_check_integrity
  • rtsk_variant_iterator_init, rtsk_variant_iterator_next, and TreeSequence$variants

#' ts_file <- system.file("examples/test.trees", package = "RcppTskit")
#' tc <- tc_load(ts_file)
#' tc$sort()
sort = function(edge_start = 0L, no_check_integrity = FALSE) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@LynxJinyangii I am comparing this R implementation against Python implementation at https://tskit.dev/tskit/docs/latest/python-api.html#tskit.TableCollection.sort and see that we have edge_start = 0L and no_check_integrity = FALSE, while Python version has edge_start=0, *, site_start=0, mutation_start=0. We must strive for parity of implementation, unless there is a strong need for deviation. Is there a reason you deviated from the Python version?

@gregorgorjanc
Copy link
Copy Markdown
Member

@LynxJinyangii I have pushed changes to this branch/PR (rebased to main, which required sorting some merge conflicts; and come comments on sort), so best if you first pull before you start changes on your end.

@gregorgorjanc
Copy link
Copy Markdown
Member

I have done some polishing of the code, docs, and terms in my recent commit&push. @LynxJinyangii make sure you git pull on your end so that you get these changes. I have reviewed all but variant/iteration stuff - that will take me more time - but you can work on the comments I provided above in the meantime and then we iterate.

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.

2 participants