Skip to content

Refactor table collection integrity checks and document. #709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jeromekelleher merged 4 commits into from
Jul 15, 2020
Merged

Refactor table collection integrity checks and document. #709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9561415
Refactor table collection integrity checks and document.
jeromekelleher Jul 8, 2020
4757193
Detect bad tree topologies at load time.
jeromekelleher Jul 10, 2020
15305e9
Move tree checks into table collection.
jeromekelleher Jul 10, 2020
2a99090
Add version to circleCI cache key.
jeromekelleher Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ jobs:
- checkout
- run: sudo chown -R circleci:circleci *
- restore_cache:
key: tskit-{{ .Branch }}
# It's sometimes necessary to nuke the cache, and the simplest
# way to do it is to change the key. We can increment this
# version number when we want to do this.
key: tskit-{{ .Branch }}-v1
- run:
name: Checkout submodules
command: |
Expand All @@ -26,7 +29,7 @@ jobs:
pip uninstall tskit -y
echo 'export PATH=/home/circleci/.local/bin:$PATH' >> $BASH_ENV
- save_cache:
key: tskit-{{ .Branch }}
key: tskit-{{ .Branch }}-v1
paths:
- "/home/circleci/.local"

Expand Down
5 changes: 5 additions & 0 deletions c/CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ In development.
- The text dump of tables with metadata now includes the metadata schema as a header.
(:user:`benjeffery`, :pr:`493`)

- Bad tree topologies are detected earlier, so that it is no longer possible
to create a tsk_treeseq_t object which contains a parent with contradictory
children on an interval. Previously an error occured when some operation
building the trees was attempted (:user:`jeromekelleher`, :pr:`709`).

**New features**

- Mutations now have an optional double-precision floating-point ``time`` column.
Expand Down
105 changes: 82 additions & 23 deletions c/tests/test_tables.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* MIT License
*
* Copyright (c) 2019 Tskit Developers
* Copyright (c) 2019-2020 Tskit Developers
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -3233,8 +3233,7 @@ test_dump_load_unsorted_with_options(tsk_flags_t tc_options)
CU_ASSERT_EQUAL_FATAL(ret, 3);

/* Verify that it's unsorted */
ret = tsk_table_collection_check_integrity(
&t1, TSK_CHECK_OFFSETS | TSK_CHECK_EDGE_ORDERING);
ret = tsk_table_collection_check_integrity(&t1, TSK_CHECK_EDGE_ORDERING);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_EDGES_NOT_SORTED_PARENT_TIME);

/* Indexing should fail */
Expand Down Expand Up @@ -3337,8 +3336,7 @@ test_dump_fail_no_file(void)
CU_ASSERT_EQUAL_FATAL(ret, 3);

/* Verify that it's unsorted */
ret = tsk_table_collection_check_integrity(
&t1, TSK_CHECK_OFFSETS | TSK_CHECK_EDGE_ORDERING);
ret = tsk_table_collection_check_integrity(&t1, TSK_CHECK_EDGE_ORDERING);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_EDGES_NOT_SORTED_PARENT_TIME);

/* Make sure the file doesn't exist beforehand. */
Expand Down Expand Up @@ -3690,7 +3688,7 @@ test_table_collection_check_integrity_with_options(tsk_flags_t tc_options)
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_mutation_table_add_row(&tables.mutations, 0, 1, 0, 0, NULL, 0, NULL, 0);
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_table_collection_check_integrity(&tables, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_ORDERING);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_MUTATION_PARENT_EQUAL);

ret = tsk_mutation_table_clear(&tables.mutations);
Expand All @@ -3704,31 +3702,31 @@ test_table_collection_check_integrity_with_options(tsk_flags_t tc_options)
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_ORDERING);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_MUTATION_PARENT_AFTER_CHILD);
ret = tsk_table_collection_check_integrity(
&tables, TSK_CHECK_MUTATION_ORDERING | TSK_NO_CHECK_MUTATION_PARENTS);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);

ret = tsk_mutation_table_clear(&tables.mutations);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_mutation_table_add_row(
&tables.mutations, 0, 1, TSK_NULL, 0, NULL, 0, NULL, 0);
&tables.mutations, 0, 1, TSK_NULL, TSK_UNKNOWN_TIME, NULL, 0, NULL, 0);
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_mutation_table_add_row(&tables.mutations, 1, 1, 0, 0, NULL, 0, NULL, 0);
ret = tsk_mutation_table_add_row(
&tables.mutations, 1, 1, 0, TSK_UNKNOWN_TIME, NULL, 0, NULL, 0);
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_ORDERING);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_MUTATION_PARENT_DIFFERENT_SITE);
ret = tsk_table_collection_check_integrity(&tables, TSK_NO_CHECK_MUTATION_PARENTS);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);

ret = tsk_mutation_table_clear(&tables.mutations);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_mutation_table_add_row(
&tables.mutations, 1, 1, TSK_NULL, 0, NULL, 0, NULL, 0);
&tables.mutations, 1, 1, TSK_NULL, TSK_UNKNOWN_TIME, NULL, 0, NULL, 0);
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_mutation_table_add_row(
&tables.mutations, 0, 1, TSK_NULL, 0, NULL, 0, NULL, 0);
&tables.mutations, 0, 1, TSK_NULL, TSK_UNKNOWN_TIME, NULL, 0, NULL, 0);
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
Expand All @@ -3742,17 +3740,35 @@ test_table_collection_check_integrity_with_options(tsk_flags_t tc_options)
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_TIME);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_ORDERING);
CU_ASSERT_EQUAL_FATAL(ret, 0);

/* Add a chain of unknown times in between a mutation and one with a
* younger time. */
ret = tsk_mutation_table_clear(&tables.mutations);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_mutation_table_add_row(
&tables.mutations, 0, 0, TSK_NULL, 1.0, NULL, 0, NULL, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_mutation_table_add_row(
&tables.mutations, 0, 0, 0, TSK_UNKNOWN_TIME, NULL, 0, NULL, 0);
CU_ASSERT_EQUAL_FATAL(ret, 1);
ret = tsk_mutation_table_add_row(
&tables.mutations, 0, 0, 1, TSK_UNKNOWN_TIME, NULL, 0, NULL, 0);
CU_ASSERT_EQUAL_FATAL(ret, 2);
ret = tsk_mutation_table_add_row(&tables.mutations, 0, 0, 2, 2.0, NULL, 0, NULL, 0);
CU_ASSERT_EQUAL_FATAL(ret, 3);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_ORDERING);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_MUTATION_TIME_OLDER_THAN_PARENT_MUTATION);

ret = tsk_mutation_table_clear(&tables.mutations);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_mutation_table_add_row(
&tables.mutations, 0, 0, TSK_NULL, NAN, NULL, 0, NULL, 0);
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_TIME);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_TIME_NONFINITE);

ret = tsk_mutation_table_clear(&tables.mutations);
Expand All @@ -3761,8 +3777,6 @@ test_table_collection_check_integrity_with_options(tsk_flags_t tc_options)
&tables.mutations, 0, 0, TSK_NULL, INFINITY, NULL, 0, NULL, 0);
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_TIME);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_TIME_NONFINITE);

ret = tsk_mutation_table_clear(&tables.mutations);
Expand All @@ -3772,7 +3786,7 @@ test_table_collection_check_integrity_with_options(tsk_flags_t tc_options)
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_TIME);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_ORDERING);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_MUTATION_TIME_YOUNGER_THAN_NODE);

ret = tsk_mutation_table_clear(&tables.mutations);
Expand All @@ -3784,12 +3798,9 @@ test_table_collection_check_integrity_with_options(tsk_flags_t tc_options)
CU_ASSERT_FATAL(ret >= 0);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_TIME);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_MUTATION_ORDERING);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_MUTATION_TIME_OLDER_THAN_PARENT_MUTATION);

/* Note that we do not test for TSK_ERR_MUTATION_TIME_OLDER_THAN_PARENT_NODE as
that is only checked for tree sequences not tables */

/* migrations */
ret = tsk_population_table_add_row(&tables.populations, NULL, 0);
CU_ASSERT_FATAL(ret >= 0);
Expand Down Expand Up @@ -3863,6 +3874,52 @@ test_table_collection_check_integrity_with_options(tsk_flags_t tc_options)
tsk_table_collection_free(&tables);
}

static void
test_table_collection_check_integrity_no_populations(void)
{
int ret;
tsk_treeseq_t ts;
tsk_table_collection_t tables;

tsk_treeseq_from_text(&ts, 10, paper_ex_nodes, paper_ex_edges, NULL, paper_ex_sites,
paper_ex_mutations, paper_ex_individuals, NULL, 0);
ret = tsk_treeseq_copy_tables(&ts, &tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);

/* Add in some bad population references and check that we can use
* TSK_NO_CHECK_POPULATION_REFS with TSK_CHECK_TREES */
tables.nodes.population[0] = 10;
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_POPULATION_OUT_OF_BOUNDS);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_TREES);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_POPULATION_OUT_OF_BOUNDS);
ret = tsk_table_collection_check_integrity(&tables, TSK_NO_CHECK_POPULATION_REFS);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(
&tables, TSK_CHECK_TREES | TSK_NO_CHECK_POPULATION_REFS);
/* CHECK_TREES returns the number of trees */
CU_ASSERT_EQUAL_FATAL(ret, 3);
tables.nodes.population[0] = TSK_NULL;

ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_migration_table_add_row(
&tables.migrations, 0.4, 0.5, 1, 0, 1, 1.5, NULL, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_POPULATION_OUT_OF_BOUNDS);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_TREES);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_POPULATION_OUT_OF_BOUNDS);
ret = tsk_table_collection_check_integrity(&tables, TSK_NO_CHECK_POPULATION_REFS);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(
&tables, TSK_CHECK_TREES | TSK_NO_CHECK_POPULATION_REFS);
CU_ASSERT_EQUAL_FATAL(ret, 3);

tsk_table_collection_free(&tables);
tsk_treeseq_free(&ts);
}

static void
test_table_collection_check_integrity(void)
{
Expand Down Expand Up @@ -4069,6 +4126,8 @@ main(int argc, char **argv)
{ "test_column_overflow", test_column_overflow },
{ "test_table_collection_check_integrity",
test_table_collection_check_integrity },
{ "test_table_collection_check_integrity_no_populations",
test_table_collection_check_integrity_no_populations },
{ "test_table_collection_subset", test_table_collection_subset },
{ "test_table_collection_subset_errors", test_table_collection_subset_errors },
{ NULL, NULL },
Expand Down
82 changes: 17 additions & 65 deletions c/tests/test_trees.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ static void
verify_compute_mutation_times(tsk_treeseq_t *ts)
{
int ret;
size_t j;
size_t size = tsk_treeseq_get_num_mutations(ts) * sizeof(tsk_id_t);
tsk_id_t *time = malloc(size);
tsk_table_collection_t tables;
Expand All @@ -140,8 +141,10 @@ verify_compute_mutation_times(tsk_treeseq_t *ts)
ret = tsk_treeseq_copy_tables(ts, &tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
memcpy(time, tables.mutations.time, size);
/* Make sure the tables are actually updated */
memset(tables.mutations.time, 0, size);
/* Time should be set to TSK_UNKNOWN_TIME before computing */
for (j = 0; j < size; j++) {
tables.mutations.time[j] = TSK_UNKNOWN_TIME;
}

ret = tsk_table_collection_compute_mutation_times(&tables, NULL, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
Expand Down Expand Up @@ -2136,17 +2139,18 @@ test_simplest_bad_indexes(void)
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_TABLES_NOT_INDEXED);
ret = tsk_table_collection_build_index(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_ALL);
CU_ASSERT_EQUAL_FATAL(ret, 0);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_TREES);
/* TSK_CHECK_TREES returns the number of trees */
CU_ASSERT_EQUAL_FATAL(ret, 1);

for (j = 0; j < sizeof(bad_indexes) / sizeof(*bad_indexes); j++) {
tables.indexes.edge_insertion_order[0] = bad_indexes[j];
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_ALL);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_TREES);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_EDGE_OUT_OF_BOUNDS);
tables.indexes.edge_insertion_order[0] = 0;

tables.indexes.edge_removal_order[0] = bad_indexes[j];
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_ALL);
ret = tsk_table_collection_check_integrity(&tables, TSK_CHECK_TREES);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_EDGE_OUT_OF_BOUNDS);
tables.indexes.edge_removal_order[0] = 0;
}
Expand Down Expand Up @@ -2361,7 +2365,6 @@ test_simplest_contradictory_children(void)
"0 1 2 0\n";
tsk_treeseq_t ts;
tsk_table_collection_t tables;
tsk_tree_t tree;
int ret;
tsk_flags_t load_flags = TSK_BUILD_INDEXES;

Expand All @@ -2375,13 +2378,8 @@ test_simplest_contradictory_children(void)
tables.sequence_length = 1.0;

ret = tsk_treeseq_init(&ts, &tables, load_flags);
CU_ASSERT_EQUAL(ret, 0);
ret = tsk_tree_init(&tree, &ts, 0);
CU_ASSERT_EQUAL(ret, 0);
ret = tsk_tree_first(&tree);
CU_ASSERT_EQUAL(ret, TSK_ERR_BAD_EDGES_CONTRADICTORY_CHILDREN);
CU_ASSERT_EQUAL_FATAL(ret, TSK_ERR_BAD_EDGES_CONTRADICTORY_CHILDREN);

tsk_tree_free(&tree);
tsk_treeseq_free(&ts);
tsk_table_collection_free(&tables);
}
Expand Down Expand Up @@ -3758,12 +3756,12 @@ test_single_tree_compute_mutation_parents(void)
const char *sites = "0 0\n"
"0.1 0\n"
"0.2 0\n";
const char *mutations = "0 0 1 -1 0\n"
"1 1 1 -1 0\n"
"2 4 1 -1 1\n"
"2 1 0 2 0\n"
"2 1 1 3 0\n"
"2 2 1 -1 0\n";
const char *mutations = "0 0 1 -1\n"
"1 1 1 -1\n"
"2 4 1 -1\n"
"2 1 0 2 \n"
"2 1 1 3 \n"
"2 2 1 -1\n";
tsk_treeseq_t ts;
tsk_table_collection_t tables;

Expand Down Expand Up @@ -5128,51 +5126,6 @@ test_offset_trees_with_errors_kc(void)
tsk_treeseq_free(&other);
}

static void
test_invalid_first_tree_errors_kc(void)
{
const char *nodes = "1 0 -1\n"
"1 0 -1\n"
"0 1 -1\n";
const char *edges = "0 1 2 0\n"
"0 1 2 1\n";

const char *bad_nodes = "1 0 -1\n"
"1 1 -1\n"
"0 1 -1\n";
const char *bad_edges = "0 1 1 0\n"
"0 1 2 0\n";
tsk_treeseq_t ts, other;
tsk_table_collection_t tables;
double result;
int ret;
tsk_flags_t load_flags = TSK_BUILD_INDEXES;

tsk_treeseq_from_text(&ts, 1, nodes, edges, NULL, NULL, NULL, NULL, NULL, 0);

ret = tsk_table_collection_init(&tables, 0);
CU_ASSERT_EQUAL_FATAL(ret, 0);

parse_nodes(bad_nodes, &tables.nodes);
CU_ASSERT_EQUAL_FATAL(tables.nodes.num_rows, 3);
parse_edges(bad_edges, &tables.edges);
CU_ASSERT_EQUAL_FATAL(tables.edges.num_rows, 2);
tables.sequence_length = 1.0;

ret = tsk_treeseq_init(&other, &tables, load_flags);
CU_ASSERT_EQUAL(ret, 0);

ret = tsk_treeseq_kc_distance(&ts, &other, 0, &result);
CU_ASSERT_EQUAL(ret, TSK_ERR_BAD_EDGES_CONTRADICTORY_CHILDREN);

ret = tsk_treeseq_kc_distance(&other, &ts, 0, &result);
CU_ASSERT_EQUAL(ret, TSK_ERR_BAD_EDGES_CONTRADICTORY_CHILDREN);

tsk_treeseq_free(&ts);
tsk_treeseq_free(&other);
tsk_table_collection_free(&tables);
}

/*=======================================================
* Miscellaneous tests.
*======================================================*/
Expand Down Expand Up @@ -5938,7 +5891,6 @@ main(int argc, char **argv)
{ "test_unequal_sequence_lengths_kc", test_unequal_sequence_lengths_kc },
{ "test_different_number_trees_kc", test_different_number_trees_kc },
{ "test_offset_trees_with_errors_kc", test_offset_trees_with_errors_kc },
{ "test_invalid_first_tree_errors_kc", test_invalid_first_tree_errors_kc },

/* Misc */
{ "test_tree_errors", test_tree_errors },
Expand Down
3 changes: 3 additions & 0 deletions c/tskit/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,9 @@ tsk_strerror_internal(int err)
case TSK_ERR_COLUMN_OVERFLOW:
ret = "Table column too large; cannot be more than 2**32 bytes.";
break;
case TSK_ERR_TREE_OVERFLOW:
ret = "Too many trees; cannot be more than 2**31.";
break;
case TSK_ERR_METADATA_DISABLED:
ret = "Metadata is disabled for this table, so cannot be set";
break;
Expand Down
Loading