Adding variants iterator and other functions by LynxJinyangii · Pull Request #132 · HighlanderLab/RcppTskit

LynxJinyangii · 2026-04-01T14:59:39Z

No description provided.

codecov · 2026-04-01T15:36:09Z

Codecov Report

❌ Patch coverage is 98.55453% with 11 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
RcppTskit/R/Class-TableCollection.R	97.22%	6 Missing ⚠️
RcppTskit/src/RcppTskit.cpp	99.10%	3 Missing ⚠️
RcppTskit/R/Class-TreeSequence.R	97.29%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

gregorgorjanc · 2026-04-11T08:49:12Z

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

gregorgorjanc · 2026-04-11T08:57:36Z

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

gregorgorjanc · 2026-04-11T09:45:17Z

@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 · 2026-04-13T05:36:54Z

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.

…e of both 1 and 1L

gregorgorjanc · 2026-05-04T20:37:39Z

@bryo-han do you have capacity to go over this PR while I focus on teaching/course?

gregorgorjanc

@LynxJinyangii I have tagged you in a bunch of places where I don't see changes when I last reviewed certain parts. Remember to click on the > or v symbol next to file names to fold and unfold the file so you can see comments as you scroll through the files. Also, when the diff is large GitHub will not render the diff so you have to click on the file display to see diffs and comment sections.

gregorgorjanc · 2026-04-12T07:38:39Z

 }

+// PUBLIC, wrapper for tsk_treeseq_get_samples
+// @describeIn rtsk_treeseq_summary Get sample node IDs.


I don't see any documentation about rtsk_treeseq_get_samples in rtsk_treeseq_summary, so we should address this. The C method is described at https://tskit.dev/tskit/docs/latest/c-api.html#c.tsk_treeseq_get_samples. Since it only accepts tsk_treeseq_t (tree sequence type in C) it does make sense to describe rtsk_treeseq_get_samples in rtsk_treeseq_summary, but we should follow the convention established there for other methods (provide a link etc.).

@LynxJinyangii

I am looking at this again and on reflection think that this method tsk_treeseq_get_samples likely should not be documented in rtsk_treeseq_summary and in fact is not called by rtsk_treeseq_summary whereas other tsk_treeseq_get_* (and similar) methods are called by rtsk_treeseq_summary. So, we should create a more extensive documentation for tsk_treeseq_get_samples here - can you follow documentation of other PUBLIC methods that have their own documentation?

gregorgorjanc · 2026-04-12T20:27:31Z

    .Call(`_RcppTskit_tskit_version`)
 }

+rtsk_const_tsk_no_check_integrity <- function() {


Since Python sort method does not have options I assume we do not need rtsk_const_tsk_no_check_integrity at all?

gregorgorjanc · 2026-04-15T20:12:47Z

+// @title Add a row to the migration table in a table collection
+// @param tc an external pointer to table collection as a
+//   \code{tsk_table_collection_t} object.
+// @param left numeric scalar left coordinate for the new migration.


Mention inclusive/exclusive

@LynxJinyangii

gregorgorjanc · 2026-04-15T20:14:00Z

Mention inclusive/exclusive

@LynxJinyangii

gregorgorjanc · 2026-04-15T20:14:32Z

+// (n_before <- RcppTskit:::rtsk_table_collection_get_num_migrations(tc_xptr))
+// (m_before <-
+// RcppTskit:::rtsk_table_collection_metadata_length(tc_xptr)$migrations) new_id
+// <- RcppTskit:::rtsk_migration_table_add_row(


As in R side - let's do a meaningful example

@LynxJinyangii

gregorgorjanc · 2026-05-04T20:40:55Z

  `pointer` to `xptr`.
 - Ensured `TableCollection$tree_sequence()` matches `tskit Python` API:
  it now builds indexes on the `TableCollection`, if indexes are not present.
+- Improved table row getter APIs and documentation for better `tskit C`/Python


@LynxJinyangii I think we can remove 78-80 since getters have been introduced as part of 0.3.0 anyway.

gregorgorjanc · 2026-05-04T20:47:13Z

      )
    },

+    # TODO: how should we handle useR's experience with numeric&integer in getters?


@LynxJinyangii remove these TODOs please - we have now decided we support both integers and numerics and that we will work with 0-based indexing (I assume)

gregorgorjanc · 2026-08-04T13:10:37Z

+    SEXP ts, Rcpp::Nullable<Rcpp::IntegerVector> samples = R_NilValue,
+    bool isolated_as_missing = true,
+    Rcpp::Nullable<Rcpp::CharacterVector> alleles = R_NilValue,
+    double left = 0.0, double right = NA_REAL);


@LynxJinyangii Python API has None for all of these args so would be good to be consistent/in-line with that API here to. The Python API then checks the inputs and does what it does. Best to follow that style on our end too. So, put R_NilValue (or should it be NA_REAL?). We can come back to this once I review everything;)

gregorgorjanc · 2026-08-04T13:11:20Z

                                int options = 0);
 SEXP rtsk_treeseq_copy_tables(SEXP ts, int options = 0);
 SEXP rtsk_treeseq_init(SEXP tc, int options = 0);
+SEXP rtsk_variant_iterator_init(


@LynxJinyangii Python API uses variants so best to follow that nomenclature.

Other functions here are named as rtsk_treeseq_smth so this should then be one of rtsk_treeseq_variants (this matches Python, but we are here at C++ level so we can deviate from Python if this makes sense!),
rtsk_treeseq_variants_init (this is clearer in action/verb), rtsk_treeseq_variants_iterator_init (this one says what we are actually initialising),
rtsk_treeseq_variants_init_iterator or maybe better rtsk_treeseq_init_variants_iterator. I personally like the last one most;0

gregorgorjanc · 2026-08-04T13:12:28Z

+    bool isolated_as_missing = true,
+    Rcpp::Nullable<Rcpp::CharacterVector> alleles = R_NilValue,
+    double left = 0.0, double right = NA_REAL);
+SEXP rtsk_variant_iterator_next(SEXP iterator);


@LynxJinyangii Python API uses variants (with s) so best to follow that nomenclature.

We need a better name too. Options?

We have rtsk_treeseq_init_variants_iterator for the initialiser. We could do something like that, as in long, but rtsk_treeseq_next_variant seems simpler and clear, so let's go with that.

gregorgorjanc · 2026-08-04T13:41:47Z

 bool rtsk_table_collection_has_index(SEXP tc, int options = 0);
 void rtsk_table_collection_build_index(SEXP tc, int options = 0);
 void rtsk_table_collection_drop_index(SEXP tc, int options = 0);
+void rtsk_table_collection_sort(SEXP tc, int edge_start = 0, int site_start = 0,


C API calls this arg as start and passes in a bookmark, which is a structure with edges, nodes, etc. members, but not all are working and Pyhon only implements 3 args listed here. We won't implement the bookmark on our end at this point. So let's call these arg on our end start_edges, start_sites, and start_mutations so we match C and C++ API, but keep these arg names on R side to match Python. I know this is super annoying, but this is the pattern we follow everywhere - C++ matches C and R matches Python. Super annoying!

gregorgorjanc · 2026-08-04T13:50:20Z

 void rtsk_table_collection_drop_index(SEXP tc, int options = 0);
+void rtsk_table_collection_sort(SEXP tc, int edge_start = 0, int site_start = 0,
+                                int mutation_start = 0, int options = 0);
+Rcpp::IntegerVector rtsk_table_collection_simplify(


Add node_map too?

Aha, no need as node_map is return from this function, right?

gregorgorjanc · 2026-08-05T09:49:21Z

+  `TableCollection$simplify()` to simplify table collections, with C-level
+  options at low level and Python-style arguments at the R6 level.
+- Added low-level variant iterators
+  (`rtsk_variant_iterator_init()`/`rtsk_variant_iterator_next()`) and a


Sync the names of these functions

gregorgorjanc · 2026-08-05T09:51:01Z

@@ -40,7 +40,31 @@ and releases adhere to [Semantic Versioning](https://semver.org/spec/v2.0.0.html
 - Added `rtsk_mutation_table_add_row()` and
  `TableCollection$mutation_table_add_row()` to append mutation rows from
  \code{R}, mirroring `tsk_mutation_table_add_row()`.


Suggested change

\code{R}, mirroring `tsk_mutation_table_add_row()`.

`R`, mirroring `tsk_mutation_table_add_row()`.

gregorgorjanc · 2026-08-05T09:51:18Z

- TODO
+- Added `rtsk_population_table_add_row()` and
+  `TableCollection$population_table_add_row()` to append population rows from
+  \code{R}, mirroring `tsk_population_table_add_row()`.


Suggested change

\code{R}, mirroring `tsk_population_table_add_row()`.

`R`, mirroring `tsk_population_table_add_row()`.

gregorgorjanc · 2026-08-05T09:51:29Z

+  \code{R}, mirroring `tsk_population_table_add_row()`.
+- Added `rtsk_migration_table_add_row()` and
+  `TableCollection$migration_table_add_row()` to append migration rows from
+  \code{R}, mirroring `tsk_migration_table_add_row()`.


Suggested change

\code{R}, mirroring `tsk_migration_table_add_row()`.

`R`, mirroring `tsk_migration_table_add_row()`.

gregorgorjanc · 2026-08-05T09:51:45Z

+  \code{R}, mirroring `tsk_migration_table_add_row()`.
+- Added `rtsk_provenance_table_add_row()` and
+  `TableCollection$provenance_table_add_row()` to append provenance rows from
+  \code{R}, mirroring `tsk_provenance_table_add_row()`.


Suggested change

\code{R}, mirroring `tsk_provenance_table_add_row()`.

`R`, mirroring `tsk_provenance_table_add_row()`.

gregorgorjanc · 2026-08-05T09:54:14Z

+- Added `rtsk_table_collection_sort()` and `TableCollection$sort()` to sort
+  table collections.
+- Added `rtsk_table_collection_simplify()` and
+  `TableCollection$simplify()` to simplify table collections, with C-level


Suggested change

`TableCollection$simplify()` to simplify table collections, with C-level

`TableCollection$simplify()` to simplify table collection.

gregorgorjanc · 2026-08-05T09:54:58Z

+- Added `rtsk_node_table_get_row()` and `TableCollection$node_table_get_row()`
+  to retrieve node-table rows by 0-based row index.
+- Added `rtsk_table_collection_sort()` and `TableCollection$sort()` to sort
+  table collections.


Suggested change

table collections.

table collection.

gregorgorjanc · 2026-08-05T09:55:13Z

+  table collections.
+- Added `rtsk_table_collection_simplify()` and
+  `TableCollection$simplify()` to simplify table collections, with C-level
+  options at low level and Python-style arguments at the R6 level.


Suggested change

options at low level and Python-style arguments at the R6 level.

gregorgorjanc · 2026-08-05T09:56:04Z

+- Added low-level variant iterators
+  (`rtsk_variant_iterator_init()`/`rtsk_variant_iterator_next()`) and a
+  user-facing `TreeSequence$variants()` method to iterate over decoded
+  site-by-site variants from \code{R}, aligned with `tskit` Python API


Suggested change

site-by-site variants from \code{R}, aligned with `tskit` Python API

site-by-site variants from `R`, aligned with `Python` API

This was referenced Apr 6, 2026

Do we have to add TableCollection$sort() method? #99

Closed

Add variants on pr 130 #131

Closed

gregorgorjanc changed the title ~~Add multiple functions on pr 131~~ Adding variants iterator and other functions Apr 10, 2026

Add variants, table row helpers, and related tests

c309e3a

gregorgorjanc force-pushed the add-multiple-functions-on-pr-131 branch from c443f64 to c309e3a Compare April 10, 2026 10:24

gregorgorjanc reviewed Apr 11, 2026

View reviewed changes

Comment thread RcppTskit/R/Class-TableCollection.R Outdated

Review/comments on TableCollection sort()

8bb475d

Only effective use of const and polish ts

094183a

gregorgorjanc mentioned this pull request Apr 12, 2026

Implement *_table_get_row #136

Open

Polishing terminology and code

ccaf32a

gregorgorjanc and others added 9 commits April 13, 2026 11:42

Unit test for content of node_table_get_row

fef7582

make TableCollection() aligned with Python version

db0b90b

add tests

e33fc2d

add add_row() functions for population migration provenance table

352d7e5

add relaxed validator validate_integer_numeric_scalar_arg to allow us…

b343f23

…e of both 1 and 1L

polish validators and add get_row() functions for other tables

0602237

add tests

fdd40d0

polish based on comments in 0602237

6a74096

polish based on comments in fdd40d0

7ec71f3

gregorgorjanc requested changes May 4, 2026

View reviewed changes

LynxJinyangii requested review from bryo-han and hannesbecher June 1, 2026 13:09

polish and add a few of tests

67f3173

LynxJinyangii removed the request for review from hannesbecher July 6, 2026 13:46

LynxJinyangii removed the request for review from bryo-han July 6, 2026 13:46

Add simplify() for table collection

2e3bf2c

gregorgorjanc reviewed Aug 4, 2026

View reviewed changes

gregorgorjanc reviewed Aug 5, 2026

View reviewed changes

	\code{R}, mirroring `tsk_mutation_table_add_row()`.
	`R`, mirroring `tsk_mutation_table_add_row()`.

	\code{R}, mirroring `tsk_population_table_add_row()`.
	`R`, mirroring `tsk_population_table_add_row()`.

	\code{R}, mirroring `tsk_migration_table_add_row()`.
	`R`, mirroring `tsk_migration_table_add_row()`.

	\code{R}, mirroring `tsk_provenance_table_add_row()`.
	`R`, mirroring `tsk_provenance_table_add_row()`.

	`TableCollection$simplify()` to simplify table collections, with C-level
	`TableCollection$simplify()` to simplify table collection.

	site-by-site variants from \code{R}, aligned with `tskit` Python API
	site-by-site variants from `R`, aligned with `Python` API

Uh oh!

Conversation

LynxJinyangii commented Apr 1, 2026

Uh oh!

codecov Bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gregorgorjanc commented Apr 11, 2026

Uh oh!

gregorgorjanc commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gregorgorjanc commented Apr 11, 2026

Uh oh!

gregorgorjanc commented Apr 13, 2026

Uh oh!

gregorgorjanc commented May 4, 2026

Uh oh!

gregorgorjanc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 1, 2026 •

edited

Loading

gregorgorjanc commented Apr 11, 2026 •

edited

Loading

gregorgorjanc Aug 5, 2026 •

edited

Loading