Skip to content

Commit 1b577fb

Browse files
committed
Record pause point for PR 132 review
1 parent ac89284 commit 1b577fb

3 files changed

Lines changed: 79 additions & 4 deletions

File tree

PR132_REVIEW_PLAN.md

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,43 @@
11
# PR #132 API Review Plan
22

3+
## Pause handoff (2026-08-21)
4+
5+
The review is deliberately paused at the start of variant iteration while
6+
Gregor is away and until the intended user-facing design has been discussed
7+
with Jinyang. This is a clean review boundary, not an indication that the
8+
variant implementation has been approved.
9+
10+
Completed review slices are node and remaining table-row access, table-row
11+
appenders, tree-sequence sample access, table-collection sorting, and table-
12+
collection/tree-sequence simplification. Their detailed notes are retained in
13+
the companion review-plan files.
14+
15+
The next work is **section 7, low-level variant iteration**. Resume with
16+
iterator state, initialization, ownership, and cleanup in
17+
`PR132_REVIEW_PLAN_variants.md`; then review decoding and exhaustion; only then
18+
review the user-facing `TreeSequence$variants()` API. Do not restart the review
19+
from the beginning of the PR.
20+
21+
Before continuing, ask Jinyang for:
22+
23+
- concise user-facing documentation for the intended variants workflow;
24+
- one or two executable examples, including expected returned content and the
25+
AlphaSimR use case;
26+
- the intended public API and compatibility requirements, particularly which
27+
iterator methods and arguments downstream code needs.
28+
29+
The local review branch was clean at commit `ac89284` before this handoff note
30+
was added. At that point it contained ten review commits beyond PR #132's head
31+
commit `24509d1`, and the latest commit had not yet been pushed to Gregor's
32+
`origin/lynx-clean-work`. PR #132 is open from
33+
`LynxJinyangii:add-multiple-functions-on-pr-131` into
34+
`HighlanderLab:main`; pushing this review branch to Gregor's fork does not by
35+
itself update the PR.
36+
37+
After variants are agreed and reviewed, complete section 9 cross-cutting
38+
cleanup and all final quality gates. No final full-package quality-gate result
39+
is claimed by this pause handoff.
40+
341
## Goal
442

543
Review and polish PR #132 function by function before it is merged into

PR132_REVIEW_PLAN_variants.md

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,44 @@
22

33
## Status
44

5-
In progress. The low-level iterator-state documentation and ownership model are
6-
under review. Continue with the finalizer and initializer, then review decoding
7-
and exhaustion, and only then review `TreeSequence$variants()`.
5+
Paused on 2026-08-21 pending discussion with Jinyang. The low-level iterator-
6+
state documentation and ownership model have been mapped, but the variant
7+
implementation and user-facing API have not been approved. There are not yet
8+
sufficient user-facing docs or examples to resolve the API by review alone.
9+
10+
When work resumes, continue with the finalizer and initializer, then review
11+
decoding and exhaustion, and only then review `TreeSequence$variants()`. Do not
12+
restart from earlier PR slices, which are recorded as complete in
13+
`PR132_REVIEW_PLAN.md`.
14+
15+
### Questions for Jinyang before resuming
16+
17+
- What concrete workflow should `TreeSequence$variants()` support, especially
18+
in AlphaSimR?
19+
- Can he provide one or two small executable examples with expected site,
20+
allele, genotype, sample-order, and missing-data results?
21+
- What returned R object and iteration protocol does he intend? Are both
22+
`next()` and `next_variant()` required?
23+
- Are the `copy` and deprecated `impute_missing_data` compatibility arguments
24+
required by downstream code, or should this new R API omit unsupported or
25+
deprecated compatibility surface?
26+
- Are the low-level iterator functions intended to be public installed C++ API
27+
or internal implementation details?
28+
- What behavior is intended for `NULL` versus empty `samples` and `alleles`,
29+
interval bounds, fixed allele mappings, and isolated/missing samples?
30+
31+
### Exact restart point
32+
33+
1. Resolve the questions above and record intentional deviations from Python.
34+
2. Resume at **C++ iterator state and initialization** below.
35+
3. Confirm finalizer and partial-initialization cleanup semantics from the
36+
upstream C API before changing ownership code.
37+
4. Remove the redundant `ts_sexp` preservation only together with regression
38+
tests covering garbage collection, iterator lifetime, early abandonment,
39+
and absence of double-free behavior.
40+
5. Complete decoding/exhaustion and exact-content tests before reviewing the R
41+
iterator API, documentation, and examples.
42+
6. Run the focused and full completion gates in section 5.
843

944
This review covers:
1045

RcppTskit/notes_pkg_dev.Rmd

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
## Next TODOs
44

5+
### Review and polish variants work
6+
57
struct rtsk_variant_iterator_state_t
68

79
static void rtsk_variant_iterator_free
@@ -41,7 +43,7 @@ One important issue surfaced while documenting this: variant_initialized is set
4143

4244
rtsk_variant_iterator_init mention copy is not implemented (a note from my postit)!?
4345

44-
// TODO: This will go into AlphaSimR
46+
### This type of code will go into AlphaSimR
4547

4648
Tskit examples of building tree sequence in C
4749
extern/tskit/c/tests/testlib.c has lots of examples of constructing a tree sequence using C

0 commit comments

Comments
 (0)