make subset and canonicalise deal with individual parents #1226

petrelharp · 2021-02-28T15:25:33Z

As discussed in #1199, the plan is for

subset to not reorder individuals
canonicalise to sort on (num descendants, first referring node)

WIP.

petrelharp · 2021-02-28T18:57:29Z

This would close #1223.

petrelharp · 2021-03-14T04:32:07Z

Well, this is bothersome: I get some VCF-related errors:

____________________________________________________________________________ TestRecordsEqual.test_simple_infinite_sites_random_ploidy ____________________________________________________________________________

tests/test_vcf.py:401: in verify
    pysam_records = list(bcf_file)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   OSError: unable to parse next record

pysam/libcbcf.pyx:4108: OSError
---------------------------------------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------------------------------------
[E::vcf_parse_format] Number of columns at 1:0 does not match the number of samples (4 vs 5)
============================================================================================= short test summary info =============================================================================================
FAILED tests/test_vcf.py::TestRecordsEqual::test_simple_infinite_sites_random_ploidy - OSError: unable to parse next record

that are apparently triggered by having individual parents - removing these two lines from tsutil.insert_random_ploidy_individuals makes the errors go away.

Edit: oh, I see - it's not actually because of the individual parents, it's just because it changed the randomness, and we happened to get a ploidy-zero individual, i.e., one without any nodes.

codecov · 2021-03-14T05:11:55Z

Codecov Report

Merging #1226 (f48a9ee) into main (fbba717) will decrease coverage by 0.00%.
The diff coverage is 93.28%.

@@            Coverage Diff             @@
##             main    #1226      +/-   ##
==========================================
- Coverage   93.76%   93.76%   -0.01%     
==========================================
  Files          26       26              
  Lines       21798    21907     +109     
  Branches      962      962              
==========================================
+ Hits        20439    20541     +102     
- Misses       1318     1325       +7     
  Partials       41       41

Flag	Coverage Δ
c-tests	`92.49% <93.28%> (+<0.01%)`	⬆️
lwt-tests	`92.97% <ø> (ø)`
python-c-tests	`94.99% <ø> (+<0.01%)`	⬆️
python-tests	`98.65% <ø> (ø)`

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

Impacted Files	Coverage Δ
python/tskit/metadata.py	`98.60% <ø> (ø)`
c/tskit/tables.c	`90.93% <93.28%> (+0.03%)`	⬆️
python/_tskitmodule.c	`91.45% <0.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbba717...f48a9ee. Read the comment docs.

petrelharp · 2021-03-14T05:54:59Z

I think this is ready for review - @benjeffery?

jeromekelleher

Looks great, thanks for working all this out @petrelharp! Some minor comments.

c/tskit/tables.c

c/tskit/tables.h

petrelharp · 2021-03-14T23:41:49Z

Codecov is complaining, but not for a good reason, I think.

jeromekelleher

Love it, looks great. A couple of minor clarification points on naming things, but good to merge after squash I think.

jeromekelleher · 2021-03-15T08:40:10Z

c/tskit/tables.c

 static int
-tsk_table_sorter_sort_individuals(tsk_table_sorter_t *self)
+tsk_individual_table_count_descendants(
+    tsk_individual_table_t *self, tsk_size_t *num_descendants, tsk_id_t *traversal_order)


This is nitpicking, but we'd normally put any optional arguments last - you'd never pass in NULL for traversal_order, right?

jeromekelleher · 2021-03-15T08:41:32Z

c/tskit/tables.c

+        ret = TSK_ERR_NO_MEMORY;
+        goto out;
+    }
+    memset(new_id_map, 0xff, num_individuals * sizeof(tsk_id_t));


Might as well use sizeof(*new_id_map)) here for consistency with mallocs above

jeromekelleher · 2021-03-15T08:44:07Z

c/tskit/tables.c


 static int
-tsk_table_sorter_sort_individuals(tsk_table_sorter_t *self)
+tsk_individual_table_count_descendants(


Hmm, it's not just counting descendents though. How about tsk_individual_table_topological_sort? This returns a topological sorting of the individuals and optionally a count of their descendents.

c/tskit/tables.c

benjeffery

Brilliant stuff! Once JKs comments are resolved we can merge and release!

c/tskit/tables.c

benjeffery · 2021-03-15T11:56:13Z

python/tests/test_tables.py

            assert tables2.mutations.num_rows == 0

+    def test_doesnt_reorder_individuals(self):
+        tables = wf.wf_sim(N=5, ngens=5, num_pops=2, seed=123, record_individuals=True)


Record individuals is now the default since #1238

Thought so, but I didn't see it... oh, I need to rebase.

python/tskit/metadata.py

petrelharp · 2021-03-15T15:43:18Z

There's some kind of bug; I'm tracking it down.

petrelharp · 2021-03-15T20:59:45Z

python/tests/tsutil.py

    ):
-        individuals.parents[i] = ind_map[individuals.parents[i]]
+        new_parents[i] = ind_map[individuals.parents[i]]
+    individuals.parents = new_parents


doh!!! This was the error.

Gah, that looks like my bad, sorry! I thought this would have been caught by the tests. Also a good argument for allowing assignment in the tables api.

petrelharp · 2021-03-15T21:01:00Z

python/tests/test_tables.py

+    def verify_union_consistency(self, tables, other, node_mapping):
+        ts1 = tsutil.insert_unique_metadata(tables)
+        ts2 = tsutil.insert_unique_metadata(other, offset=1000000)
+        for tsk in [False, True]:


This is checking both the tskit union and py_union for consistency; we don't need to do that now that they both pass, I guess - but I did this to find the error in py_union.

These are great and close #1243

To really close that one we should tests mutations and sites too; I've done this and updated.

petrelharp · 2021-03-15T21:02:27Z

I did a much more thorough test for consistency of union; it took longer than expected, but it's a good test. Hopefully everything passes, now?

benjeffery · 2021-03-16T01:00:34Z

All looks good, will merge and release tomorrow!

canonicalise sorts on (num descendants, earliest referring node)

jeromekelleher

LGTM, let's merge and ship!

This was referenced Feb 28, 2021

Sort individuals #1199

Merged

subset (and maybe other methods?) need to remap individual.parent #1223

Closed

This was referenced Mar 8, 2021

Remove inconsistant mutation error #1233

Merged

union needs to remap individual parent IDs #1225

Closed

petrelharp force-pushed the subset_indivs branch 3 times, most recently from c3be9ec to 5efbb17 Compare March 13, 2021 19:50

petrelharp marked this pull request as ready for review March 14, 2021 05:54

jeromekelleher reviewed Mar 14, 2021

View reviewed changes

c/tskit/tables.c Outdated Show resolved Hide resolved

c/tskit/tables.c Show resolved Hide resolved

c/tskit/tables.c Outdated Show resolved Hide resolved

c/tskit/tables.c Show resolved Hide resolved

c/tskit/tables.h Show resolved Hide resolved

jeromekelleher approved these changes Mar 15, 2021

View reviewed changes

benjeffery approved these changes Mar 15, 2021

View reviewed changes

petrelharp force-pushed the subset_indivs branch from 3f8717e to 1e173f0 Compare March 15, 2021 14:23

petrelharp commented Mar 15, 2021

View reviewed changes

petrelharp added 2 commits March 15, 2021 20:21

subset does not reorder individuals

85297db

canonicalise sorts on (num descendants, earliest referring node)

union consistency check and py_union fix: closes tskit-dev#1243

f48a9ee

petrelharp force-pushed the subset_indivs branch from 9ab6723 to f48a9ee Compare March 16, 2021 03:21

jeromekelleher approved these changes Mar 16, 2021

View reviewed changes

benjeffery added the AUTOMERGE-REQUESTED label Mar 16, 2021

mergify bot merged commit d911160 into tskit-dev:main Mar 16, 2021

mergify bot removed the AUTOMERGE-REQUESTED label Mar 16, 2021

petrelharp deleted the subset_indivs branch March 17, 2021 04:04

make subset and canonicalise deal with individual parents #1226

make subset and canonicalise deal with individual parents #1226

Uh oh!

Conversation

petrelharp commented Feb 28, 2021

Uh oh!

petrelharp commented Feb 28, 2021

Uh oh!

petrelharp commented Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

petrelharp commented Mar 14, 2021

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrelharp commented Mar 14, 2021

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!

Uh oh!

benjeffery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petrelharp commented Mar 15, 2021

Uh oh!

petrelharp Mar 15, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrelharp commented Mar 15, 2021

Uh oh!

benjeffery commented Mar 16, 2021

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

petrelharp commented Mar 14, 2021 •

edited

Loading

codecov bot commented Mar 14, 2021 •

edited

Loading

petrelharp Mar 15, 2021 •

edited

Loading