put binary derived state into the mutation table metadata

[bhaller]

This PR fixes #664 (I hope). Review requested from @petrelharp. Right now I've tested that it round-trips a tree sequence out to disk and back, and that seems to work AFAICT. Let me know if it does what you need it to do.

Also, note: it does what it does in an unorthodox way, by munging around inside the tskit data structures, because I didn't want to be making copies of this stuff – the derived state column could potentially be a pretty big buffer, so I didn't want to push the high-water memory usage mark higher if possible. If there is a cleaner and more orthodox way to do what I'm doing, that would be great of course. In any case, please make sure that the way that I'm munging tskit's data structures is completely correct and safe (given the current tskit implementation). This code ought to be bulletproof, of course, but I'm not sure that it is since I don't feel 100% confident in my understanding of how tskit does things under the hood (and AFAIK that is not documented anywhere, even in comments, right?).

[bhaller]

And again GitHub Desktop has based this PR on master instead of multitrait. I definitely told the GUI to base it off of multitrait; I was careful! So, hmm, annoying. Can you fix it up again, however you do that, @petrelharp? (Is there UI in GitHub to do it, somewhere?) [EDIT: Found the UI, which is quite hidden! You have to click the "Edit title" button, and having done that, a popup appears that lets you choose the branch the PR will be merged into. Their UI designers were smoking something when they came up with that! :-O]

[codecov]

Codecov Report

❌ Patch coverage is 94.49541% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.90%. Comparing base (3d2c471) to head (c98f248).
⚠️ Report is 6 commits behind head on multitrait.

Files with missing lines	Patch %	Lines
core/species.cpp	94.49%	6 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff               @@
##           multitrait     #665      +/-   ##
==============================================
+ Coverage       76.86%   76.90%   +0.04%     
==============================================
  Files             117      117              
  Lines           78747    83275    +4528     
  Branches        14299    16165    +1866     
==============================================
+ Hits            60528    64044    +3516     
- Misses          18219    19231    +1012     

Files with missing lines	Coverage Δ
core/slim_globals.cpp	69.25% <ø> (+0.16%)	⬆️
core/species.h	91.80% <ø> (ø)
core/species.cpp	69.58% <94.49%> (+4.84%)	⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bhaller]

Hmm. The branch that this PR is based on, derived_state_metadata, still appears to be based on master. So maybe this is still screwed up. I'm going to just delete this PR, and its associated branch, and try again. Grr.

[bhaller]

OK, clearly I'm (yet again) fundamentally confused by Git's conceptual model. I just made a new branch, derived_state_metadata_2, based on multitrait. I published that branch. If I now look at it in GitHub, it says "This branch is 335 commits ahead of master." I was expecting it to say "This branch is 0 commits ahead of multitrait." Why is it comparing itself to master?? But if I go to Terminal and do git reflog show derived_state_metadata_2, it says "Created from multitrait", and it looks like HEAD for derived_state_metadata_2 points to the current head of multitrait. Aargh, so confusing. So I'm going to hypothesize that the original derived_state_metadata branch was OK, and that once I switched this PR over to being based against master (why didn't it base off of multitrait, since it knows that derived_state_metadata is a branch off of multitrait??), now this PR is also OK. So I'm going to reopen this PR, and delete my derived_state_metadata_2 branch. But it'd be helpful to understand why Git is acting this way, and to be reassured that merging this PR will not somehow corrupt the project. Grr, grr.

[petrelharp]

The github-ness of this PR looks good - it's based on multitrait, and shows 1 commit. Github's display tells you that the branch is way ahead of master, because there isn't a way to say that a branch is supposed to be compared to a given other branch; it's only a PR that has a notion of "branch to compare to".

[petrelharp]

I haven't got into the weeds on the main work here yet because I think there's a much simpler way to do this. The derived_state and metadata columns are stored in exactly the same way by tskit. So, why not just move around the pointers instead of looping over entries? So we want to move the pointers metadata and metadata_length over to derived_state and derived_state_length, and then set metadata to NULL and metadata_length to 0?

(Tangentially, this made me think "why not just go through the code and (more or less) change over the references to derived_state to metadata?" But this is no good because we make heavy use of the tsk_variant object, which is very useful and relies on ancestral/derived states.)

[petrelharp]

Ah also I think we want "we don't actually use derived_state for anything, but make it an ASCII copy of the comma-separated list of IDs" as a policy, which isn't reflected in the comments here, but should be? (We should also update treerec/implementation.md?)

[bhaller]

I haven't got into the weeds on the main work here yet because I think there's a much simpler way to do this. The derived_state and metadata columns are stored in exactly the same way by tskit. So, why not just move around the pointers instead of looping over entries? So we want to move the pointers metadata and metadata_length over to derived_state and derived_state_length, and then set metadata to NULL and metadata_length to 0?

That's pretty much what the code does. That's the purpose of the std::swap() calls. But I can't leave the the metadata pointer as NULL, I think? All the columns are supposed to be allocated, aren't they? That was my understanding, but like I said, I am not aware of ANY documentation on how the policies these internal data structures are supposed to follow. It's all just in Jerome and Ben's head (and yours, if you understand it all). It drives me crazy, their total opposition to commenting code. Anyhow, if you look at the code for Species::DerivedStatesFromMetadata() it does exactly what you suggest, just with a 1-byte allocation rather than NULL. Species::DerivedStatesToMetadata() is more complicated because it still has to convert the binary derived state info into ASCII.

(Tangentially, this made me think "why not just go through the code and (more or less) change over the references to derived_state to metadata?" But this is no good because we make heavy use of the tsk_variant object, which is very useful and relies on ancestral/derived states.)

Yes, the derived state column needs to be functional, I think? Maybe for simplify as well?

[bhaller]

...there isn't a way to say that a branch is supposed to be compared to a given other branch; it's only a PR that has a notion of "branch to compare to"...

Huh, how strange. When you make a branch, you make it based on another branch; and reflog knows that it is based on that other branch; and if you make a PR from it, you will of course want that PR to be compared to that other branch (at least as a default); so why not show the comparison to that other branch, then? Git knows it...? This is a very strange and confusing design decision.

[petrelharp]

That's pretty much what the code does. That's the purpose of the std::swap() calls.

Ah, good.

But I can't leave the the metadata pointer as NULL, I think? All the columns are supposed to be allocated, aren't they?

Oh, you're probably right.

That was my understanding, but like I said, I am not aware of ANY documentation on how the policies these internal data structures are supposed to follow.

Have you read the C API docs? It's kinda minimal but it is SOME documentation.

[petrelharp]

Also: I think the reason that the sort of documentation you're looking for here doesn't exist is because we haven't settled on a public and stable API for it. We want to have the freedom to change how some of this works under the hood.

[bhaller]

Have you read the C API docs? It's kinda minimal but it is SOME documentation.

I've skimmed it and found it pretty useless for my purposes. For example, on the salient question of whether you're allowed to free a column and put NULL there, with a zero length, I don't see that it says anything at all. Or, when I put a 1-byte malloced pointer there instead, since I was afraid NULL isn't allowed, so I do:

	free(p_tables->mutations.metadata);
	p_tables->mutations.metadata = (char *)malloc(1);	// avoids platform-dependencies on zero-length malloc
	...
	p_tables->mutations.metadata_length = 0;
	p_tables->mutations.max_metadata_length = 1;

Is that allowed? I have no idea. What are the rules and requirements for how columns are managed? I don't see any guidance on that at all. (If I have missed it somewhere, then of course I'll eat my words; but I've mentioned this before and have gotten nowhere.)

[bhaller]

Also: I think the reason that the sort of documentation you're looking for here doesn't exist is because we haven't settled on a public and stable API for it. We want to have the freedom to change how some of this works under the hood.

I don't think that's a valid reason not to document code. You can document it and write "This is internal and subject to change." I've had that debate with Jerome and Ben. :->

[bhaller]

OK, accepted/replied/resolved review stuff. I'll now work on improving the code with tsk_mutation_table_takeset_columns() as suggested. :->

[bhaller]

OK, updated with tsk_mutation_table_takeset_columns() as discussed. (Although I followed a slightly different strategy than you proposed, to avoid needing to malloc the temporary mutation table or swap it in.) A final review would be great, and then hopefully this is ready to merge. Thanks!

[petrelharp]

Why do you need those swaps above? Why not just

Suggested change

	mutation_table.derived_state,
	mutation_table.derived_state_offset,
	mutation_table.metadata,
	mutation_table.metadata_offset,

[petrelharp]

oh wait reading the comment now

[petrelharp]

Wait, no, I don't get it - why is all this swapping necessary? Why can't you do:

1. takeset to the temporary table, with the metadata in the derived_state slot
2. set p_tables->mutations to the (pointer to the) temporary table
3. free the original table's metadata schema?

[bhaller]

I don't like changing p_tables->mutations to the temporary table, for a couple of reasons. (1) It requires that the temporary table be malloced, so it adds an allocation call (which is slow) and a free. (2) Changing over to a new pointer adds a risk of bugs where someone has a pointer to the table and now their pointer is invalid. So I prefer the way I've written it. I want the temporary table to remain the temporary table, and to fix up the original table to be the way it ought to be.

[petrelharp]

FYI, and probably you know this, but the current code does a malloc, since takeset calls takeset_ragged_column which calls alloc_empty_ragged_column for the empty columns.

[bhaller]

Yeah; no way I can prevent that, with tskit's APIs. (It surprises me that they chose to make empty columns still be malloced rather than nullptr; that's not the choice I would've made.)

[petrelharp]

okay, and this is more confusing here: why swap -> free schema -> swap back? what are the swaps doing?

[bhaller]

The second set of swaps is not a "swap back"; it is swapping the derived state and metadata columns within the single mutation table. That is necessary to put things where they belong, since the binary derived state information was being kept in the derived state column, and now needs to be in the metadata column (and vice versa). I think the code is correct and necessary as it stands (given that I don't want to change the pointer to the mutation table in the table collection); have another look.

[petrelharp]

Hah, sorry yes.

[petrelharp]

BTW: are you sure that std::swap actually doesn't copy the contents? I see that maybe std::vector::swap doesn't, but is this a "vector"? (Not that this is important that we don't copy, btw.)

[bhaller]

BTW: are you sure that std::swap actually doesn't copy the contents? I see that maybe std::vector::swap doesn't, but is this a "vector"? (Not that this is important that we don't copy, btw.)

Yep, I'm quite sure of that.

[bhaller]

Merging after code review and Slack discussion.