Tskit: More fine-grained integrity check flags would be helpful

[bhaller]

I'd like to make SLiM call tsk_table_collection_check_integrity() in each generation, to ensure that bugs like https://github.com/MesserLab/SLiM/issues/62 don't slip through the cracks again. However, the set of flags presently supported by tsk_table_collection_check_integrity() don't provide fine-grained enough control. For example:

• While simulation is running in SLiM, we have no population table set up. We therefore always fail the check population >= num_populations and get a TSK_ERR_POPULATION_OUT_OF_BOUNDS error. This check is always run (there is no flag controlling it), so there's no way for us to turn it off. So at present we can't call tsk_table_collection_check_integrity() at all.

• Some of what TSK_CHECK_MUTATION_ORDERING controls would be useful to us. However, mutation ordering itself is not maintained while SLiM is running; we do a sort immediately before simplifying, to fix that up. So dividing TSK_CHECK_MUTATION_ORDERING into the parts we want, versus the sort-order check we don't want, would be useful.

There are probably other cases as well. @petrelharp says he will have a go at this, since he understands both the tskit side and the SLiM side of this issue. Once the necessary changes are made in tskit, they would be pulled into SLiM's copy of the code. SLiM now has a stub method, SLiMSim::CheckTreeSeqIntegrity(), where a call to tsk_table_collection_check_integrity() is commented out; that stub is just waiting for this fix, and then can be enabled with the appropriate flags.

[bhaller]

Might interest @molpopgen as well

[molpopgen]

Can you get by with just subsets of flags as appropriate?

[bhaller]

If the existing flags get subdivided more finely, plus the automatic tests that are presently not controllable by any flag get put under appropriate flags as well (so that the function does nothing at all if passed flags of 0, ideally), that would probably do the trick. I'm not requesting brand-new tests here (although if that happened, that would of course be great too).

[molpopgen]

I think I get it--you have non NULL population values for nodes, but no population table until later, etc.? I'm a bit new to some of this myself. fwdpp uses its own scheme for the tree sequence recording, and a few points are different.

[bhaller]

I think all our population values are 0 in the tskit tables, because we have no population table. But the code checks population >= num_populations, and num_populations is zero, so the check fails. One possible fix for that would be to do the population >= num_populations iff num_populations > 0; don't enforce population index checks if the population table is empty/nonexistent. But if that's not a good idea, then make it controllable with a flag.

[petrelharp]

I think all our population values are 0 in the tskit tables, because we have no population table.

Oh! If that's so, then we should be setting population to -1.

[bhaller]

I think all our population values are 0 in the tskit tables, because we have no population table.

Oh! If that's so, then we should be setting population to -1.

Hmm, I just ran with a breakpoint on the TSK_ERR_POPULATION_OUT_OF_BOUNDS failure. I think @molpopgen's surmise was correct, actually; we're setting population indices according to SLiM's indices, but the population table just doesn't exist. So they're not 0, necessarily; they are whatever the pop id in SLiM is, by the look of it.

[petrelharp]

That makes more sense.

[bhaller]

Spoke too hastily. :->

[jeromekelleher]

See proposal in #596 where we allow opting out of population reference integrity checking.