Documentation/Consistency Review #3329

benjeffery · 2025-11-20T13:45:46Z

I thought it wise before 1.0 to do a full (automated) review that documentation lines up with the code we're running. Several hours of token crunching and 45min of human review/improvement resulted in these commits, which are best viewed as separate diffs as it's a bit of a grab-bag. I think these are all worth merging though - even if they are things we weren't planning to get to.

That there wasn't more than this to find is very reassuring, but then a lot of effort has been put in to ensure tskit's quality.

codecov · 2025-11-20T13:51:30Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.72%. Comparing base (5ffcf6f) to head (32ae37f).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3329   +/-   ##
=======================================
  Coverage   89.72%   89.72%           
=======================================
  Files          29       29           
  Lines       31181    31192   +11     
  Branches     5720     5722    +2     
=======================================
+ Hits        27976    27988   +12     
  Misses       1796     1796           
+ Partials     1409     1408    -1

Flag	Coverage Δ
c-tests	`86.71% <100.00%> (+<0.01%)`	⬆️
lwt-tests	`80.38% <ø> (ø)`
python-c-tests	`87.05% <ø> (ø)`
python-tests	`98.84% <100.00%> (+<0.01%)`	⬆️
python-tests-no-jit	`33.51% <7.69%> (-0.05%)`	⬇️
python-tests-numpy1	`50.16% <92.30%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
c/tskit/tables.c	`83.08% <100.00%> (+0.01%)`	⬆️
python/tskit/trees.py	`98.89% <100.00%> (+<0.01%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jeromekelleher

I've gone through and it all looks good to me except for the IBDsegments stuff, which I'd have to spend more time on. I'd suggest pulling this commit out into it's own PR, and get the rest merged.

jeromekelleher · 2025-11-20T14:24:30Z

docs/stats.md

+statistics, isolated samples without mutations directly above them are treated
+as carrying the ancestral allele rather than as missing. Future versions of
+tskit may expose options to treat missing data differently in statistics; for
+now, if you need explicit control over how missing data are handled you should


Suggested change

now, if you need explicit control over how missing data are handled you should

now, if you need explicit control over how missing data is handled you should

benjeffery · 2025-11-20T14:31:48Z

IBD extracted to #3330

benjeffery · 2025-11-20T14:50:22Z

I'll wait for @petrelharp before merging.

petrelharp · 2025-11-21T17:29:39Z

docs/data-model.md

 requirements for each edge are:

- We must have {math}`0 \leq` `left` {math}`<` `right` {math}`\leq L`;
+- We must have finite coordinates with {math}`0 \leq` `left` {math}`<` `right` {math}`\leq L`;


This is redundant, but it's fine; this is supposed to be careful and specific. (you can't have a non-finite value x for which 0 <= x < L with L finite)

petrelharp · 2025-11-21T17:29:58Z

docs/data-model.md


 #### Migration requirements

 Given a valid set of nodes and edges, the requirements for a value set of


Suggested change

Given a valid set of nodes and edges, the requirements for a value set of

Given a valid set of nodes and edges, the requirements for a valid set of

petrelharp · 2025-11-21T17:35:33Z

docs/export.md

-on the VCF parser, and so we do **not** account for this change in
-coordinate system by default.
+Tree sequence site positions can be floating point values, whereas VCF
+requires discrete, 1-based integers. The ``position_transform`` argument


Suggested change

requires discrete, 1-based integers. The ``position_transform`` argument

requires positive integers. The ``position_transform`` argument

petrelharp · 2025-11-21T17:35:52Z

docs/export.md

-coordinate system by default.
+Tree sequence site positions can be floating point values, whereas VCF
+requires discrete, 1-based integers. The ``position_transform`` argument
+controls how tskit maps coordinates into VCF. Translating non-integral


Suggested change

controls how tskit maps coordinates into VCF. Translating non-integral

controls how tskit maps coordinates into VCF. Translating non-integer

petrelharp · 2025-11-21T17:39:33Z

docs/export.md

+integer, so multiple sites may share the same output position. Because VCF
+parsers differ, we do **not** automatically shift from tskit's 0-based
+convention to VCF's 1-based convention.


Suggested change

integer, so multiple sites may share the same output position. Because VCF

parsers differ, we do **not** automatically shift from tskit's 0-based

convention to VCF's 1-based convention.

integer, so multiple sites may share the same output position.

Furthermore, tskit's coordinate system starts at zero,

whereas the VCF standard requires positions to be positive,

and so a site at position 0 is not valid in the VCF standard.

Because VCF parsers differ, we do **not** do anything to account for this.

petrelharp · 2025-11-21T17:54:36Z

docs/export.md

+:::{warning}
+Most VCF tools expect POS and contig coordinates to be 1-based. If your tree
+sequence already uses discrete, 0-based integer positions, leaving the default
+transform will produce off-by-one coordinates in the VCF. Supply an explicit
+``position_transform`` (for example, add 1 after rounding) or use the
+``"legacy"`` option to shift to 1-based coordinates.
+:::


I am not sure about how this is written. I think there are different interpretations of what's going on here:

"0-based / 1-based" - if I'm interpreting correctly, this means that tskit coordinates are "distance to the right of the left end of the chromosome" and VCF is "which number site, counting from the left end of the chromosome".

"valid values" - tskit allows a site at coordinate 0, VCF does not

Under interpretation (1), the coordinates are indeed off by one. However, I think that the simpler interpretation (2) should be the preferred interpretation, because then they're in the same coordinate system and there's no possibility to get mixed up with off-by-one errors.

Here's a rather pedantic suggestion for clarifying the situation.

Suggested change

:::{warning}

Most VCF tools expect POS and contig coordinates to be 1-based. If your tree

sequence already uses discrete, 0-based integer positions, leaving the default

transform will produce off-by-one coordinates in the VCF. Supply an explicit

``position_transform`` (for example, add 1 after rounding) or use the

``"legacy"`` option to shift to 1-based coordinates.

:::

The simplest resolution of this discrepancy in convention between tskit and VCF

positions is deal with any site at position 0 as a special case (for instance,

by discarding or ignoring it).

A different interpretation of this difference between tskit's position

and VCF's POS field

is that they are different coordinate systems: tskit coordinates are

"distance to the right of the left end of the chromosome",

while VCF coordinates are "which number site, counting from the left end

of the chromosome and starting at one".

Under this interpretation, the solution is to supply an explicit

``position_transform`` that adds 1 to the coordinate when outputting

to VCF (or, using the ``"legacy"`` option described below). However, this can

easily lead to off-by-one errors converting between the coordinate systems,

so should only be used if you really are using 0-based coordinates for your

tree sequence.

:::{warning}

Most VCF tools cannot deal with a POS value of 0. If your tree

sequence contains a site with position 0, this will likely cause an error.

:::

petrelharp · 2025-11-21T17:55:36Z

docs/export.md

+For example, to shift all coordinates by 1 to make them strictly
+1-based, we could define:


Suggested change

For example, to shift all coordinates by 1 to make them strictly

1-based, we could define:

For example, to shift all coordinates by 1, we could define:

I'm removing "1-based" because I don't think that term is unambiguous.

petrelharp · 2025-11-21T17:57:52Z

docs/file-formats.md

-`UNKNOWN_TIME` will be used to fill the column. See the
+more than one mutation occurs at the same site). If the `time` column
+is absent, the mutation times in the resulting tree sequence are set to
+{data}`tskit.UNKNOWN_TIME`. Unknown mutation times written out by


Suggested change

{data}`tskit.UNKNOWN_TIME`. Unknown mutation times written out by

{data}`tskit.UNKNOWN_TIME`, which is a numeric value that behaves like NaN.

Unknown mutation times written out by

petrelharp · 2025-11-21T17:59:08Z

This all looks good, but see comments/suggestions?

benjeffery added 4 commits November 20, 2025 12:09

Check table sequence length is finite

7ffb4ff

Clarify data model and validity documentation

6446d2f

Document VCF export behaviour and limits

062d861

Clarify file-format compatibility and text output

6338451

jeromekelleher reviewed Nov 20, 2025

View reviewed changes

benjeffery force-pushed the doc-review branch from 95bad61 to fe6a5fb Compare November 20, 2025 14:29

benjeffery added 3 commits November 20, 2025 14:30

Clarify site statistics handling of missing data

e332a3b

Support metadata decoding in parse_edges

83ff801

Fix Tree and TreeSequence docstrings

32ae37f

benjeffery force-pushed the doc-review branch from fe6a5fb to 32ae37f Compare November 20, 2025 14:30

benjeffery mentioned this pull request Nov 20, 2025

Improve IBD documentation and IdentitySegments docs #3330

Open

jeromekelleher approved these changes Nov 20, 2025

View reviewed changes

jeromekelleher requested a review from petrelharp November 20, 2025 14:35

benjeffery added this pull request to the merge queue Nov 20, 2025

benjeffery removed this pull request from the merge queue due to a manual request Nov 20, 2025

petrelharp reviewed Nov 21, 2025

View reviewed changes

	now, if you need explicit control over how missing data are handled you should
	now, if you need explicit control over how missing data is handled you should


		#### Migration requirements

		Given a valid set of nodes and edges, the requirements for a value set of

	requires discrete, 1-based integers. The ``position_transform`` argument
	requires positive integers. The ``position_transform`` argument

	controls how tskit maps coordinates into VCF. Translating non-integral
	controls how tskit maps coordinates into VCF. Translating non-integer

-:::{warning}
-Most VCF tools expect POS and contig coordinates to be 1-based. If your tree
-sequence already uses discrete, 0-based integer positions, leaving the default
-transform will produce off-by-one coordinates in the VCF. Supply an explicit
-``position_transform`` (for example, add 1 after rounding) or use the
-``"legacy"`` option to shift to 1-based coordinates.
-:::
+The simplest resolution of this discrepancy in convention between tskit and VCF
+positions is deal with any site at position 0 as a special case (for instance,
+by discarding or ignoring it).
+A different interpretation of this difference between tskit's position
+and VCF's POS field
+is that they are different coordinate systems: tskit coordinates are
+"distance to the right of the left end of the chromosome",
+while VCF coordinates are "which number site, counting from the left end
+of the chromosome and starting at one".
+Under this interpretation, the solution is to supply an explicit
+``position_transform`` that adds 1 to the coordinate when outputting
+to VCF (or, using the ``"legacy"`` option described below). However, this can
+easily lead to off-by-one errors converting between the coordinate systems,
+so should only be used if you really are using 0-based coordinates for your
+tree sequence.
+:::{warning}
+Most VCF tools cannot deal with a POS value of 0. If your tree
+sequence contains a site with position 0, this will likely cause an error.
+:::

		For example, to shift all coordinates by 1 to make them strictly
		1-based, we could define:

	{data}`tskit.UNKNOWN_TIME`. Unknown mutation times written out by
	{data}`tskit.UNKNOWN_TIME`, which is a numeric value that behaves like NaN.
	Unknown mutation times written out by

Documentation/Consistency Review #3329

Are you sure you want to change the base?

Documentation/Consistency Review #3329

Uh oh!

Conversation

benjeffery commented Nov 20, 2025

Uh oh!

codecov bot commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jeromekelleher left a comment

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!

benjeffery commented Nov 20, 2025

Uh oh!

benjeffery commented Nov 20, 2025

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!

petrelharp Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

petrelharp commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 20, 2025 •

edited

Loading

petrelharp Nov 21, 2025 •

edited

Loading