Tskit: Should table editing functions make copies or edit in-place?

[hyanwong]

I've just noticed that while delete_intervals() and keep_intervals() return a copy of the table collection, my new delete_sites() function edits the table object in-place instead, and does not return a copy.

I'm about to code up trim() on a table collection, and it would be useful to know what the correct thing to do here is.

A hybrid approach would be to have a parameter copy=False|True for all these table-editing functions. If we are chaining various editing functions together, it makes sense to me to allow the possibility of in-place editing, otherwise I'm guessing it will be a burden for large tree sequences?

[jeromekelleher]

Good spot. I think these should be "functional" by default, i.e., they return a copy of the tables not update in place. If this turns out to be a perf bottleneck at some point we can add a in_place=False argument, or something. We don't need this now.

So, I think we should update delete_sites to return a copy. @petrelharp, opinions?

[hyanwong]

Functional is easier in some ways, yes. The counter argument is that it is easy for a user to work on copies by doing table.copy() first, whereas it is impossible to do in-place editing unless we make the functions do so. So we restrict choice by making them functional, but leave user flexibility when doing this in-place.

Am I right in thinking that tables don't have a "edit" vs "read-only" mode? Otherwise I would guess that when in "edit" mode the user is likely to want to edit in-place.

[petrelharp]

Functional sounds good to me: this is what simplify() does, also. (I think - I have to go look it up most every time!)

[jeromekelleher]

this is what simplify() does, also.

Simplify and a few others operate on the tables in place, but these are performance critical. The TreeSequence is immutable, so everything is functional on that.

[hyanwong]

I'm tending to think that many of the table methods should also be implemented as functional methods on a tree sequence (e.g. ts.delete_sites(), ts.keep_intervals(), ts.trim(), etc.). I was going to code this up sometime, by simply calling the underlying table methods (so hopefully very little code duplication: the tests should be essentially identical too).

If so, then perhaps we should think of tree_sequence methods as providing the functional interface, and table methods as in-place? Of course, tables are somewhat different, as they don't have the same sorting constraints. But it would make a nice and clear distinction.

[petrelharp]

this is what simplify() does, also.
Simplify and a few others operate on the tables in place

Told you I have to go look it up every time.

Seriously though, the lack of consistency is a significant source of bugs. It'd be nice to have it be as consistent as possible. I (obviously) don't have the big picture about what is functional and what is not.

[hyanwong]

It'd be nice to have it be as consistent as possible.

Would you consider as inconsistent my suggestion of tables.function() == always in-place, ts.function() == always functional? (An honest question - I'm happy with any answer).

Perhaps useful to list the table-editing functions we talking about here: are there others?

• simplify()
• sort()
• keep/delete intervals()
• delete_sites()
• trim()
• deduplicate_sites()
• compute_mutation_parents()

Some of these are only valid for tables, as they are guaranteed not to be needed for tree sequences (I think those are sort, compute_mutation_parents, deduplicate_sites, and the _index methods I haven't listed above).

[petrelharp]

Would you consider as inconsistent my suggestion of tables.function() == always in-place,
ts.function() == always functional?

Yes! This is a good idea, but I don't know if it's consistent with the current API.

[hyanwong]

I think it's only keep_intervals() and delete_intervals() that are functional. It would obviously be a breaking change if we made these operate in-place, but since they are such a recent addition, I think it's probably only you and possibly Andrew Kern who are using them at the moment?

[petrelharp]

Well, here's the docs; there's also build/drop_index, which is also in-place (but should say so explicitly?).

So, I like that proposal - say that table collection methods will be "always" in-place, unless there's a reason and we make it real obvious somehow, and we switch keep/delete_intervals to be in-

I'm still totally going to make the mistake where I assign the output to something, but oh well.

[petrelharp]

@andrewkern do you have any input?

[hyanwong]

As I said above, as a corollary, I think that we should implement keep/delete intervals(), delete_sites(), and (when I implement it) trim() on tree sequences too. In fact, unless there is good reason otherwise, I think most operations on tables should have a functional equivalent on a tree sequence. My hope is that this might make it easier for casual (!) users to work entirely with tree sequences, and not have to know about tables unless they really want to get into the guts of tskit.

[jeromekelleher]

I like Yan's idea of tables in place an tree seq functional. Unless it breaks some long standing methods I think we should standardise on this.

[hyanwong]

I've coded this change in https://github.com/tskit-dev/tskit/pull/372. It seemed easier from a testing perspective to make the switch to in-place methods at the same time as creating the tree_seq.delete_intervals() and tree_seq.keep_intervals() methods, so those are included too. I still have to include tree_seq.delete_sites() - I could add that to this PR or create another instead.