Tskit: Mutation time - backwards compatibility

[benjeffery]

As it stands when #672 is merged we break compatibility in the following ways:

1. File format (not too bad as #672 has a tskit upgrade cli support)
2. python MutationTable.add_row every call to this function will need updating.
3. python MutationTable.[set|append]_columns every call to these functions will need updating.
4. cpython LightweightTableCollection.from_dict will fail when passed dicts made in, for example, msprime 0.7.4 or newer.
5. C API extra time args to mutation table methods (no way around this)

2 and 3 _could_ be avoided by allowing a default mutation-time of zero as on these methods. This would result in an invalid tree-sequence _but_ TableCollection.tree_sequence could catch mutation time errors, call compute_mutation_times and build the tree sequence again, logging out a warning that mutation times were set.

4 Could be avoided in a similar way, by detecting the absence of mutation times in the dict and calling compute_mutation_times again.

Note that currently compute_mutation_times spreads mutations evenly across edges (except for those above a root node where the mutation is placed at the same time as the root)

In summary we could make this release more or less backward compatible (python-wise). My concern is that users should be aware that they have placeholder mutation times, as otherwise they could end up using them in some analysis as if they were "proper".

Tagging some people who may have input here: @jeromekelleher @petrelharp @hyanwong @molpopgen @bhaller

[hyanwong]

I think we need a notation for mutation times that are unknown (apart from the fact that they occur between the parent and child node on an edge). "Unknown" could, indeed, be the default, which would therefore serve as a placeholder, no? Since the time should be floating point, using NaN seems the obvious choice to me, although I know @jeromekelleher dislikes using NaN like this, and for good reason.

[bhaller]

The C API changes can certainly be accommodated by SLiM, we'll just need to pull over the latest tskit code, make the necessary changes, and do a new release. @petrelharp generally does the pull from tskit. Once that's done, I can roll a new SLiM release quickly, usually within a day.

[jeromekelleher]

I think on balance putting NaNs in as the default is a good idea, and will save us a lot of compatibility headaches.

Unless code uses mutation times, then the presence of NaNs in this field won't affect them. Thus, all old code on old and new tree sequences will continue to work.

New code using the tables API that is run on a tree sequence that contains NaNs for time, will return an answer that contains NaNs. This seems like a reasonable way of signaling the existance of times that are unknown. If we write algorithms that process mutation times within tskit in the future, we will have to decide on a case-by-case basis whether it's better to propagate the NaNs, or to raise an error. I can see both being appropriate in different cases, so we just need to be aware of the issue, I think.

Given that all tree sequences currently have no mutation time, and some programs (like tsinfer) don't have any way of putting times on mutations (we could make them up, but it would be basically meaningless), then allowing for this slight complication in the data model seems worth it.

Once we use the built in macros for working with NaNs with a little bit of care, I think this would work fine.

[bhaller]

This would also allow you to avoid @benjeffery's point 5 ("C API extra time args to mutation table methods (no way around this)") – the existing API could be preserved, and put a NAN in for the mutation time, and a new API could be provided that has the additional mutation time argument. I don't think we care one way or the other in SLiM – we ought to migrate to the new API anyway, so that we can move the mutation times out of metadata into the mutation table. But if you want to avoid breaking other existing C code, that option exists, it seems to me. @petrelharp do you concur?

[petrelharp]

Allowing unknown values would be nice, and NaNs seem OK for this. But is the proposal is to allow them in the tables, but still not in the tree sequence? If so, I don't think the NaN functionality is going to be used, really, since it'd be a pain to keep tables around that you can't ever tree-sequence. It's also hard for me to imagine a use case where some mutation times would be known and others not, so propagating NaNs wouldn't really be helpful, since either a stat would make sense for all mtuations or none. Maybe I'm missing something?

Re: the C API - I vote to change it, no need to add a new method. (note: a code search for tsk_mutation_table_add_row turns up a small number of users we could notify about this).

[jeromekelleher]

I had imagined allowing them in the tree sequence also @petrelharp. Re the propagating NaNs from stats, I wasn't really thinking about having mixed values, but more what's useful to the user: should the stat return NaN if called on unknown mutation times, or should it raise an error? I can see both being reasonable, I guess.

Re: the C API - I vote to change it, no need to add a new method. (note: a code search for tsk_mutation_table_add_row turns up a small number of users we could notify about this).

Agreed - until we hit 1.0 I think we should try to keep the surface area of the API as small as we can.

[petrelharp]

Ok, if it'll be in the tree sequence also, then that's great. But then isn't it pretty much an optional column? I thought we decided we really did want values in here?

[jeromekelleher]

Ok, if it'll be in the tree sequence also, then that's great. But then isn't it pretty much an optional column? I thought we decided we really did want values in here?

Yeah, it is an optional column now. We did think we really wanted values in there, I think, until we started thinking through the consequences in terms of compatability. A major version bump really is a big deal, so if we can avoid it having a little ickiness in the data model is worth it.

There's also the argument that things like tsinfer really don't make mutation times that have any meaning, much, so forcing it to choose arbitrary values is more likely to end up affecting users who don't realise their mutation times are meaningless (and I can imagine putting mutations uniformly on branch could be very badly wrong in this context).

[bhaller]

Does it being an "optional column" mean that the column can literally be left out, in the tables, and on disk, and a value of NaN is assumed when the column is missing? Or once this change is made, will new files written to disk always contain a NaN value (or a proper time value) for each mutation?

[jeromekelleher]

Does it being an "optional column" mean that the column can literally be left out, in the tables, and on disk, and a value of NaN is assumed when the column is missing? Or once this change is made, will new files written to disk always contain a NaN value (or a proper time value) for each mutation?

Yes - files that are missing the mutation/time column will be assumed to contain NaN at load time. We will write out files that contain the mutation/time column, even if it is all NaNs though (but see #537 and #538 for thoughts on writing out columns that don't contain any useful information - I think it would be good to be a bit more clever about this, and it would probably save a fair bit of file storage space.)

[bhaller]

So in-memory the column will always exist, even if it is filled with NaN? (Seems reasonable, I'm just wondering exactly what you guys mean by "optional column".)

[jeromekelleher]

So in-memory the column will always exist, even if it is filled with NaN? (Seems reasonable, I'm just wondering exactly what you guys mean by "optional column".)

Yes, the in-memory column always exists (but see #585 for ways we're thinking about avoiding storage space for optional stuff like edge metadata when building tables).

[petrelharp]

Yeah, it is an optional column now. We did think we really wanted values in there, I think, until we started thinking through the consequences in terms of compatability.

Sorry, I was all turned around.

But, ok - if the column can be optional, I think providing defaults to the python functions, that defaults to whatever the "default value if the column isn't present" is, is a good idea.

[jeromekelleher]

But, ok - if the column can be optional, I think providing defaults to the python functions, that defaults to whatever the "default value if the column isn't present" is, is a good idea.

Agreed. We can default to None, which gets turned into a NaN when calling the C code (probably we should define a constant, TSK_MUTATION_TIME_UNKNOWN for this).

[molpopgen]

So, this only affects me when generating an output file. That's all set to be redone anyways when 0.3.0 hits so that the new metadata can be used

[benjeffery]

Based on the discussion here #672 now has a backwards-compatible implementation, using a specific NAN to represent unknown times, and relaxing the tree sequence validity constraints to allow this specific NAN. Note that a NAN generated from computation would fail this test.