Add sample_nodes_by_ploidy function by benjeffery · Pull Request #3157 · tskit-dev/tskit

benjeffery · 2025-05-07T12:02:13Z

No description provided.

codecov · 2025-05-07T12:10:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.59%. Comparing base (6542fc2) to head (a12aab9).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3157   +/-   ##
=======================================
  Coverage   89.58%   89.59%           
=======================================
  Files          28       28           
  Lines       31885    31895   +10     
  Branches     5855     5857    +2     
=======================================
+ Hits        28565    28575   +10     
  Misses       1888     1888           
  Partials     1432     1432

Flag	Coverage Δ
c-tests	`86.66% <ø> (ø)`
lwt-tests	`80.38% <ø> (ø)`
python-c-tests	`88.18% <ø> (ø)`
python-tests	`98.79% <100.00%> (+<0.01%)`	⬆️

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

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

🚀 New features to boost your workflow:

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

jeromekelleher

Nice! Can we do a follow-up commit where we use the new functions in the vcf() method? (Just to see if we're implementing the right interface?)

jeromekelleher · 2025-05-07T12:23:10Z

+    def sample_nodes_by_ploidy(self, ploidy):
+        """
+        Returns an 2D array of node IDs, where each row has length `ploidy`.
+        This is useful when inidividuals are not defined in the tree sequence


Suggested change

This is useful when inidividuals are not defined in the tree sequence

This is useful when individuals are not defined in the tree sequence

I sprinkle these in to prove it's not AI. Fixed.

benjeffery · 2025-05-07T13:59:37Z

I've used the new functions in the VCF code - I've tried to keep the tests unchanged with a couple of exceptions:

A more descriptive error when samples % ploidy != 0
Mixed sample and non-sample nodes are now allowed.
It is now allowed that some sample nodes are not associated with individuals.

benjeffery · 2025-05-07T14:04:19Z

Huh, test_vcf.py is passing, but I didn't check test_cli.py locally. Digging in.

benjeffery · 2025-05-07T14:11:16Z

Hmm, some of the error semantics in the existing code are a bit odd. I've replicated them for compatibility. For example an individual with no nodes is fine, but not if that individual was specifically asked for. This was only tested in the CLI code.

jeromekelleher · 2025-05-07T14:24:01Z

I think it's OK to relax the input requirements a bit now. Making things that used to raise an error work as expected now seems like forward progress to me, and it seems unlikely we'll break much code by doing that (people don't tend to rely on errors much). We just need to document the changes.

benjeffery · 2025-05-07T15:20:23Z

Ok, I've simplified things so that the error is only triggered when all the individuals specified have no nodes.

jeromekelleher · 2025-05-07T15:23:28Z

What's the output for an individual with no nodes then, missing data?

We should document these corner cases now, in the vcf method or it'll be forgotten again

benjeffery · 2025-05-08T11:57:08Z

I've added some changes to the docs and a changelog.

jeromekelleher

I don't think we've got the semantics quite right here - it would be surprising if I specified individuals=[0, 1] and I got back a VCF with zero samples in it. In this case, I think we should raise an error.

jeromekelleher · 2025-05-08T12:18:39Z

-        that is a sample and another that is not) in the data model will
-        result in an error by default. However, such individuals can be
-        excluded using the ``individuals`` argument.
+        using the ``individuals`` argument. And individual specified that


jeromekelleher · 2025-05-08T12:21:03Z


+**Changes**
+
+- Allow mixed sample and non-sample nodes to be output by ```write_vcf``,


Suggested change

- Allow mixed sample and non-sample nodes to be output by ```write_vcf``,

- Allow mixed sample and non-sample nodes to be output by ```write_vcf``.

jeromekelleher · 2025-05-08T12:23:08Z

+  and for individuals without nodes to be specified for output, although
+  they will not appear in the VCF.


Suggested change

and for individuals without nodes to be specified for output, although

they will not appear in the VCF.

Individuals without nodes no longer raise an error and are instead ignored,

except in the case where they are explicitly specified in the individuals argument.

benjeffery · 2025-05-08T12:33:01Z

Fixed up.

jeromekelleher · 2025-05-08T13:02:57Z

-        result in an error by default. However, such individuals can be
-        excluded using the ``individuals`` argument.
+        using the ``individuals`` argument. An individual specified that
+        is not associated with any nodes will be ignored and not included in the


That's not true any more

I thought it was - I added a test in 266f0cb to check.

jeromekelleher · 2025-05-08T14:00:38Z

I'm confused. I thought the semantics was:

If there's an individual in the individuals table that has no nodes, and ts.vcf(individuals=None), then that individual gets ignored in the output
If that individual is references in ts.vcf(individuals=some_list) then an error is raised

That's what the release notes say to me, but the docstring is not.

benjeffery · 2025-05-08T14:14:07Z

The code and docstring are "An individual specified that is not associated with any nodes will be ignored and not included in the output." The changelog before revisions was "allow individuals without nodes to be specified for output, although they will not appear in the VCF" which is the same thing. I didn't read your revision close enough to realise it meant the opposite. Happy to change the code if you think the erroring on nodeless, specified, individual is better.

jeromekelleher · 2025-05-08T14:53:25Z

Yes please

petrelharp · 2025-05-08T19:18:10Z

+        This is useful when individuals are not defined in the tree sequence
+        so `TreeSequence.individuals_nodes` cannot be used. The samples are
+        placed in the array in the order which they are found in the node
+        table. Ploidy must be a multiple of the number of sample nodes.


Suggested change

table. Ploidy must be a multiple of the number of sample nodes.

table. The number of sample nodes must be a multiple of ploidy.

benjeffery · 2025-05-08T22:40:08Z

Ok, have tried to be really careful this time! It is now an error again to specify nodeless individuals. I've also realised that it needs to be stated in the changelog that it is now allowed to have sample nodes that have no individual.

jeromekelleher

I think the semantics are still pretty weird - two of the tests look like the same thing to me, with different errors. See suggested new semantics.

jeromekelleher · 2025-05-09T08:13:06Z

+        with pytest.raises(ValueError, match="is not associated with any nodes"):
+            ts.as_vcf(
+                allow_position_zero=True,
+                individuals=list(range(ts.num_individuals)) + [ts.num_individuals - 1],


The last bit is redundant isn't it?

jeromekelleher · 2025-05-09T08:20:04Z

-                        "Sample nodes must either all be associated with individuals "
-                        "or not associated with any individuals"
-                    )
+        # If there are no individuals, or all the individuals are not associated with


I think we should make this simpler - just

if ploidy is not None and individuals is not None: raise ValueError("...") if individuals is None: if ts.num_individuals > 0: individuals_nodes = ts.individuals_nodes else: ploidy = 1 if ploidy is None else ploidy individuals_nodes = ts.sample_nodes_by_ploidy(ploidy) else: individuals_nodes = ts.individuals_nodes[individuals]

benjeffery · 2025-05-12T14:32:08Z

As discussed, I've removed the VCF code changes here to reduce this to solely the split_samples_by_ploidy function.

benjeffery force-pushed the split-samples branch 4 times, most recently from 7bedfae to e8fa19a Compare May 7, 2025 12:06

jeromekelleher reviewed May 7, 2025

View reviewed changes

benjeffery force-pushed the split-samples branch from e8fa19a to f3444d9 Compare May 7, 2025 12:30

benjeffery force-pushed the split-samples branch from be4e540 to 6b8848f Compare May 7, 2025 14:10

benjeffery force-pushed the split-samples branch from 6b8848f to d732eb6 Compare May 7, 2025 15:00

jeromekelleher reviewed May 8, 2025

View reviewed changes

benjeffery force-pushed the split-samples branch 2 times, most recently from 7d64905 to 754a765 Compare May 8, 2025 12:32

jeromekelleher reviewed May 8, 2025

View reviewed changes

Comment thread python/CHANGELOG.rst Outdated

jeromekelleher reviewed May 8, 2025

View reviewed changes

petrelharp reviewed May 8, 2025

View reviewed changes

benjeffery force-pushed the split-samples branch 4 times, most recently from 9a8eaec to 7bcf8a6 Compare May 8, 2025 22:34

jeromekelleher reviewed May 9, 2025

View reviewed changes

benjeffery force-pushed the split-samples branch from 7bcf8a6 to d038e9b Compare May 12, 2025 14:26

Add sample_nodes_by_ploidy function

a12aab9

benjeffery force-pushed the split-samples branch from d038e9b to a12aab9 Compare May 12, 2025 14:31

jeromekelleher approved these changes May 12, 2025

View reviewed changes

jeromekelleher added this pull request to the merge queue May 12, 2025

Merged via the queue into tskit-dev:main with commit c914b8b May 12, 2025
19 checks passed

benjeffery deleted the split-samples branch May 12, 2025 18:29

	This is useful when inidividuals are not defined in the tree sequence
	This is useful when individuals are not defined in the tree sequence


		Changes

		- Allow mixed sample and non-sample nodes to be output by ```write_vcf``,

		and for individuals without nodes to be specified for output, although
		they will not appear in the VCF.

	table. Ploidy must be a multiple of the number of sample nodes.
	table. The number of sample nodes must be a multiple of ploidy.

Conversation

benjeffery commented May 7, 2025

Uh oh!

codecov Bot commented May 7, 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!

benjeffery commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjeffery commented May 7, 2025

Uh oh!

benjeffery commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeromekelleher commented May 7, 2025

Uh oh!

benjeffery commented May 7, 2025

Uh oh!

jeromekelleher commented May 7, 2025

Uh oh!

benjeffery commented May 8, 2025

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 May 8, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeromekelleher commented May 8, 2025

Uh oh!

benjeffery commented May 8, 2025

Uh oh!

jeromekelleher commented May 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjeffery commented May 8, 2025

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!

benjeffery commented May 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented May 7, 2025 •

edited

Loading

benjeffery commented May 7, 2025 •

edited

Loading

benjeffery commented May 7, 2025 •

edited

Loading