Tskit: Valid TS should not allow nodes from multiple populations within the same individual

[hyanwong]

At the moment, it appears as if it is valid to create nodes within the same individual that belong to different populations (code below). I think we should ban this, and raise an error when tables with this property are turned into a tree sequence. Does this seem sensible? I don't think that having a single individual containing two sample nodes each belong to a different population makes much sense.

ts = msprime.simulate(4, random_seed=1)
tables = ts.tables
tables.individuals.add_row()
tables.individuals.add_row()
ind = tables.nodes.individual
pop = tables.nodes.population
ind[ts.samples()] = np.arange(ts.num_samples)//2
tables.nodes.individual = ind
pop = np.full(ts.num_nodes, tskit.NULL, dtype=tables.nodes.population.dtype)
pop[0] = 0  # only sample 0 has a population, but 
tables.nodes.population = pop
new_ts = tables.tree_sequence()
print(str(new_ts.tables.nodes))

id  flags   population  individual  time    metadata
0   1   0   0   0.00000000000000    
1   1   -1  0   0.00000000000000    
2   1   -1  1   0.00000000000000    
3   1   -1  1   0.00000000000000    
4   0   -1  -1  0.17986860883729    
5   0   -1  -1  1.97752050080185    
6   0   -1  -1  2.69754599651364    

[petrelharp]

A related potential problem is that nodes from the same individual can have different times. This is because nodes were around before individuals, so some properties that ought to be individual properties were node properties. I say we should check for these inconsistencies (and, btw, here's how I do it in pyslim) and throw an error if there are any. However, this isn't urgent.

[hyanwong]

Thanks @petrelharp . I'm just putting together the PR to check in the C routine tsk_table_collection_check_node_integrity in tables.c, which looks like the right place to me.

[hyanwong]

By the way, it's easier to check for this if #750 is assumed to be true. But I don't know if we really do want to still enforce this limitation on the order of nodes. Do you know, @petrelharp ?

[jeromekelleher]

I'm not convinced we should check for these properties tbh. What's going to break if the nodes for an individual do have different times/populations etc? We're only really enforcing aspects of the data model if we really need to internally, and given that we haven't been enforcing this up till now, there's a very good chance we'll break someone's code if we start doing so now. I

So, I'd vote to leave it as is.

[hyanwong]

The reason that I want to ensure this property is that I can then convert from tsinfer SampleData files (where the individuals have times and populations) to tree sequences, and be assured that I can map directly from one to the other.

OTOH, I can make this check when moving from ts -> sample_data, and bomb out if the tree sequence is not representable in the sample_data format. It just seemed cleaner and more logical to enforce it at the tree sequence level.

Note that if someone has managed to make a (pathological, IMO) tree sequence of this sort, it's probably a good sign that they've done something wrong, so raising an error seems sensible, and I'm more with @petrelharp here.

[petrelharp]

there's a very good chance we'll break someone's code if we start doing so now.

I dunno - if we break anyone's code because of this, isn't it certainly a bug in their code? The adjacency of nodes from the same individual is another story, though.

[jeromekelleher]

Isn't it at least possible that somehow two nodes from different populations could combine into one individual? Or with slightly different times? Why rule this out? If we insist that both nodes of an individual have to have the same population and time, then what we're saying is that our data model is wrong and we just haven't bothered fixing it. This way we're keeping some flexibility that may well come in useful at some point.

It's fine that the data models for tsinfer and tskit are slightly different - one is much more general than the other.

[hyanwong]

Isn't it at least possible that somehow two nodes from different populations could combine into one individual? Or with slightly different times?

You mean that you think a node might have a population independently of the population in which an individual sits? But then the individual would be lacking a population (and a time). That seems ... odd. I've tended to think about it as simply a redundancy in the data model.

[petrelharp]

Isn't it at least possible that somehow two nodes from different populations could combine into one individual?

Oh! Yes, I suppose so - if you think of the nodes as gametes, and their time as when they were produced, which could be quite different for the different chromosomes of moss, or other bryophytes, in which the long-lived part of the life cycle is the haploid one.

That seems ... odd.

Darn biology. =)

[petrelharp]

It does seem like a useful sanity check for non-bryophytes, at least - so, if you'd like to have it, @hyanwong, you could make it as an optional check?

[hyanwong]

In the bryophyte case, wouldn't you simply have the 2 gametes as 2 nodes, and the sporophyte as an individual containing a separate set of 2 nodes, immediately descending from the gametes? After all (a) there could be mutations between them and (b) you would probably want both 2 haploid individuals (2 gametophyte "individuals", associated with the first 2 nodes) and a single diploid sporophyte individual (associated with the latter 2 nodes). That's how I would do it, anyway. The time of the sporophyte nodes would presumably be the time of fertilisation.

[petrelharp]

You could do it that way too, but adding in the extra sporophyte nodes seems a bit odd?

I could also imagine in a tsinfer-ish application wanting to group together ancient nodes that weren't at exactly the same time but were kinda close as a tentative "individual"...

[jeromekelleher]

This basically is redundancy in the data model @hyanwong, but redundancy isn't always a bad thing (in my old job as a storage engineer redundancy is something to strive for!). My take is that either we embrace this redundancy, seeing that it might come in useful at some point or we get rid of the redundancy entirely and move population and time refs etc into the individual. I don't like the halfway house you're proposing, where we're formally acknowledging that the data model is broken. I just don't see what would be gained.

Having an optional check as @petrelharp suggests sounds like a great idea, though, as producers of tree sequences could check this as a sanity check.

[hyanwong]

Having an _optional_ check as @petrelharp suggests sounds like a great idea, though, as producers of tree sequences could check this as a sanity check.

I guess this would be an option in the tree_sequence() method, e.g. ts.tree_sequence(check_individuals=True)? I would quite like something like this to default to True, as it's very likely to be what the user intended. But I suspect you would prefer to default it to False?

[jeromekelleher]

The first thing would be to add a flag INDIVIDUAL_NODES_CONSISTENT (or similar) to the C check_integrity function. This is not part of TSK_CHECK_ALL (or it becomes part of the requirements for a tree sequence, which I disagree with). Then we'd need to expose this function through the Python C API. I guess then we'd need to add boolean parameters for all of the possible flags.

It'd be quite a bit of work seeing this through, though.

[hyanwong]

The first thing would be to add a flag INDIVIDUAL_NODES_CONSISTENT (or similar) to the C check_integrity function.

I think you mean tsk_table_collection_check_integrity() ?

This is not part of TSK_CHECK_ALL

You mean TSK_CHECK_TREES, I think?

It'd be quite a bit of work seeing this through, though.

Yes, a pain. But probably "the right way to do it". I still don't know if we want to default to performing this (optional) check when turning tables into a tree sequence.

[jeromekelleher]

I still don't know if we want to default to performing this (optional) check when turning tables into a tree sequence.

No, we definitely don't - otherwise it becomes part of the data format spec (see arguments above).

[hyanwong]

I still don't know if we want to default to performing this (optional) check when turning tables into a tree sequence.

No, we definitely don't - otherwise it becomes part of the data format spec (see arguments above).

Well, if it's the default in the python .tree_sequence() method, then I would argue that this would be a good thing to catch unexpected bugs in user code. But I sense I'm not going to convince you.

[jeromekelleher]

But I sense I'm not going to convince you.

No, I'm afraid not - I've been caught out too many times by the unexpected consequences of what seem like perfectly straightforward and reasonable changes to the format spec. Unless we have a compelling reason for changing the definition of what a valid tree sequence is, I don't think we should change it.

[jeromekelleher]

Closing this due to inactivity - unless we have a compelling reason to change the semantics here, I don't think we should.