Column takeset

[jeromekelleher]

Provide x_table_takeset_columns

Part of #2016, #1977, #537, #538

This is my initial outline for discussion, and completely untested. What do you think @benjeffery?

[codecov]

Codecov Report

Merging #2032 (c527398) into main (bb43c74) will decrease coverage by 0.20%.
The diff coverage is 90.98%.

❗ Current head c527398 differs from pull request most recent head c0ef9e4. Consider uploading reports for the commit c0ef9e4 to get more accurate results

@@            Coverage Diff             @@
##             main    #2032      +/-   ##
==========================================
- Coverage   93.33%   93.12%   -0.21%     
==========================================
  Files          27       27              
  Lines       25538    26452     +914     
  Branches     1163     1292     +129     
==========================================
+ Hits        23835    24633     +798     
- Misses       1673     1787     +114     
- Partials       30       32       +2     

Flag	Coverage Δ
c-tests	92.25% <90.98%> (-0.01%)	⬇️
lwt-tests	89.05% <ø> (ø)
python-c-tests	72.10% <ø> (+<0.01%)	⬆️
python-tests	98.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
c/tskit/tables.c	90.21% <90.98%> (-0.06%)	⬇️
python/tskit/tables.py	93.46% <0.00%> (-5.44%)	⬇️
python/_tskitmodule.c	90.96% <0.00%> (+0.03%)	⬆️
c/tskit/genotypes.c	92.58% <0.00%> (+2.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb43c74...c0ef9e4. Read the comment docs.

[benjeffery]

Looks good, shame we have to duplicate some logic but a separate function is the way.

[benjeffery]

I've rebased to master, trying this out with the kastore changes.

[benjeffery]

Added an initial test and fix. Next step is testing missing columns and bad combos.

[jeromekelleher]

Great - can you ping me when you'd like some input here please? (Would it be simpler to start your own PR or is it the same thing to just push to my branch? I don't mind)

[benjeffery]

Great - can you ping me when you'd like some input here please? (Would it be simpler to start your own PR or is it the same thing to just push to my branch? I don't mind)

My plan is to complete the proposal and tests for the IndividualTable, then get feedback. It's almost there so later today. Will push to this one if that's ok!

[benjeffery]

@jeromekelleher Had to move things about to get errors to happen before memory is changed. I think this is worth a review now.

[jeromekelleher]

LGTM! I've had a good think about how we might avoid the duplication of NULL logic between here and append_columns but I can't see how it could be done (at least not in a way that would be worse than the code duplication). Press on with this pattern would be my vote.

[benjeffery]

Great, I have commenced the great pasting ritual.

[benjeffery]

@jeromekelleher Think this is ready to review now!

[benjeffery]

Just realised we also need takeset indexes too!

[benjeffery]

@jeromekelleher Ok! I think we are good here, I've used these methods over at #2143 successfully.

[benjeffery]

Gah, missing coverage on all the ragged-column-contents error branches. Will add that in.

[benjeffery]

Ok, tests added. I think all the remaining uncovered branches are only triggerable by malloc errors.

[jeromekelleher]

Looks great! Thanks for slogging through this @benjeffery.

I think we just need to add a few tests to cover the check_ragged_column calls on the input. It should be straightforward enough to do by setting x_col_offset = -1 (or something). That will bump up the test coverage quite a bit.

[jeromekelleher]

Can you ping me when this is ready for a last look please @benjeffery?

[benjeffery]

@jeromekelleher Ok! Coverage looks a lot better.

[jeromekelleher]

Great, let's merge!

(I can't approve this as it's my PR - you'll need to approve yourself I think)