Clarify the semantics of skipping sort on sites/mutations.

Author: jeromekelleher

c/tests/test_tables.c

@@ -2978,6 +2978,81 @@ test_copy_table_collection(void)
     tsk_treeseq_free(&ts);
 }
 
+static void
+test_sort_tables_offsets(void)
+{
+    int ret;
+    tsk_treeseq_t *ts;
+    tsk_table_collection_t tables, copy;
+    tsk_bookmark_t bookmark;
+
+    ts = caterpillar_tree(10, 5, 5);
+    ret = tsk_treeseq_copy_tables(ts, &tables, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+
+    ret = tsk_table_collection_sort(&tables, NULL, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_SORT_MIGRATIONS_NOT_SUPPORTED);
+
+    tsk_migration_table_clear(&tables.migrations);
+    ret = tsk_table_collection_sort(&tables, NULL, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+
+    /* Check that setting edge offset = len(edges) does nothing */
+    reverse_edges(&tables);
+    ret = tsk_table_collection_copy(&tables, &copy, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+    memset(&bookmark, 0, sizeof(bookmark));
+    bookmark.edges = tables.edges.num_rows;
+    ret = tsk_table_collection_sort(&tables, &bookmark, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+    CU_ASSERT_FATAL(tsk_table_collection_equals(&tables, &copy));
+
+    ret = tsk_table_collection_sort(&tables, NULL, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+    CU_ASSERT_FATAL(tables.sites.num_rows > 2);
+    CU_ASSERT_FATAL(tables.mutations.num_rows > 2);
+
+    /* Check that setting mutation and site offset = to the len
+     * of the tables leaves them untouched. */
+    reverse_mutations(&tables);
+    /* Swap the positions of the first two sites, as a quick way
+     * to disorder the site table */
+    tables.sites.position[0] = tables.sites.position[1];
+    tables.sites.position[1] = 0;
+    ret = tsk_table_collection_copy(&tables, &copy, TSK_NO_INIT);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+    memset(&bookmark, 0, sizeof(bookmark));
+    bookmark.sites = tables.sites.num_rows;
+    bookmark.mutations = tables.mutations.num_rows;
+    ret = tsk_table_collection_sort(&tables, &bookmark, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, 0);
+    CU_ASSERT_FATAL(tsk_table_collection_equals(&tables, &copy));
+
+    /* Anything other than len(table) leads to an error for sites
+     * and mutations, and we can't specify one without the other. */
+    memset(&bookmark, 0, sizeof(bookmark));
+    bookmark.sites = tables.sites.num_rows;
+    ret = tsk_table_collection_sort(&tables, &bookmark, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_SORT_OFFSET_NOT_SUPPORTED);
+
+    memset(&bookmark, 0, sizeof(bookmark));
+    bookmark.mutations = tables.mutations.num_rows;
+    ret = tsk_table_collection_sort(&tables, &bookmark, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_SORT_OFFSET_NOT_SUPPORTED);
+
+    memset(&bookmark, 0, sizeof(bookmark));
+    bookmark.sites = tables.sites.num_rows - 1;
+    bookmark.mutations = tables.mutations.num_rows - 1;
+    ret = tsk_table_collection_sort(&tables, &bookmark, 0);
+    CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_SORT_OFFSET_NOT_SUPPORTED);
+
+    tsk_table_collection_free(&tables);
+    tsk_table_collection_free(&copy);
+    tsk_treeseq_free(ts);
+    free(ts);
+}
+
+
 static void
 test_sort_tables_drops_indexes_with_options(tsk_flags_t tc_options)
 {
@@ -3128,7 +3203,7 @@ test_sort_tables_errors(void)
     memset(&pos, 0, sizeof(pos));
     pos.migrations = 1;
     ret = tsk_table_collection_sort(&tables, &pos, 0);
-    CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_SORT_OFFSET_NOT_SUPPORTED);
+    CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_MIGRATIONS_NOT_SUPPORTED);
 
     memset(&pos, 0, sizeof(pos));
     pos.sites = 1;
@@ -4552,6 +4627,7 @@ main(int argc, char **argv)
             test_link_ancestors_samples_and_ancestors_overlap },
         { "test_link_ancestors_multiple_to_single_tree",
             test_link_ancestors_multiple_to_single_tree },
+        { "test_sort_tables_offsets", test_sort_tables_offsets },
         { "test_sort_tables_drops_indexes", test_sort_tables_drops_indexes },
         { "test_sort_tables_edge_metadata", test_sort_tables_edge_metadata },
         { "test_sort_tables_no_edge_metadata", test_sort_tables_no_edge_metadata },

c/tskit/core.c

@@ -351,8 +351,9 @@ tsk_strerror_internal(int err)
             ret = "Migrations not currently supported by this operation";
             break;
         case TSK_ERR_SORT_OFFSET_NOT_SUPPORTED:
-            ret = "Specifying position for mutation, sites or migrations is not "
-                  "supported";
+            ret = "Sort offsets for sites and mutations must be either 0 "
+                  "or the length of the respective tables. Intermediate values "
+                  "are not supported";
             break;
         case TSK_ERR_NONBINARY_MUTATIONS_UNSUPPORTED:
             ret = "Only binary mutations are supported for this operation";

c/tskit/tables.c

@@ -4557,7 +4557,7 @@ tsk_table_sorter_run(tsk_table_sorter_t *self, tsk_bookmark_t *start)
         edge_start = start->edges;
 
         if (start->migrations != 0) {
-            ret = TSK_ERR_SORT_OFFSET_NOT_SUPPORTED;
+            ret = TSK_ERR_MIGRATIONS_NOT_SUPPORTED;
             goto out;
         }
         /* We only allow sites and mutations to be specified as a way to

c/tskit/tables.h

@@ -2383,8 +2383,12 @@ Positions in tables that are not sorted (``individual``, ``node``, ``population`
 and ``provenance``) are ignored and can be set to arbitrary values.
 
 .. warning:: The current implementation only supports specifying a start
-    position for the ``edge`` table. Specifying a non-zero ``migration``,
-    ``site`` or ``mutation`` start position results in an error.
+    position for the ``edge`` table and in a limited form for the
+    ``site`` and ``mutation`` tables. Specifying a non-zero ``migration``,
+    start position results in an error. The start positions for the
+    ``site`` and ``mutation`` tables can either be 0 or the length of the
+    respective tables, allowing these tables to either be fully sorted, or
+    not sorted at all.
 
 The table collection will always be unindexed after sort successfully completes.
 

python/tests/test_topology.py

@@ -5698,9 +5698,9 @@ def verify(self, ts):
         # Called by the examples in ExampleTopologyMixin
         samples = ts.samples()
         self.verify_keep_input_roots(ts, samples[:2])
-        # self.verify_keep_input_roots(ts, samples[:3])
-        # self.verify_keep_input_roots(ts, samples[:-1])
-        # self.verify_keep_input_roots(ts, samples)
+        self.verify_keep_input_roots(ts, samples[:3])
+        self.verify_keep_input_roots(ts, samples[:-1])
+        self.verify_keep_input_roots(ts, samples)
 
     def verify_keep_input_roots(self, ts, samples):
         ts_with_roots, node_map = self.do_simplify(
@@ -5762,6 +5762,8 @@ def verify_keep_input_roots(self, ts, samples):
                         new_mutations = {
                             mut.metadata: mut for mut in new_site.mutations
                         }
+                        # Just make sure the metadata is actually unique.
+                        self.assertEqual(len(new_mutations), len(new_site.mutations))
                         input_site = input_sites[position]
                         for input_mutation in input_site.mutations:
                             if input_mutation.node in root_path: