opened
jeromekelleher
username_0
Seemed simplest to tackle this head on, as it's complicating quite a lot of what we're doing. It *should* close #527 (there's a test in here to verify it), but we really should get some files in from earlier file minor versions to make sure we're doing the right thing.

Closes #528
Closes #527
Closes #506

Not ready for merging yet though, as it needs some more C test coverage and there's something weird going on in one of the tests that needs figuring out.
Do not use kastore memory for columns.
issue
created
benjeffery
username_1
This looks great, so much simpler to use `set_columns` now we're copying out of `kastore`

I've installed SLiM and confirmed that @username_2 's example now works for this branch, and understand how zero edges was happening before (think I should get a `if (ret != 0)` tattoo as a penance).
comment
created
petrelharp
username_2
Oh!! Thanks for the reminder, I think the code I am working on needs a `ret !=0` or two yet.
comment
created
benjeffery
username_1
The failing test is fixed by removing `TSK_NO_INIT_TABLES` in https://github.com/tskit-dev/tskit/pull/530/commits/b2e7536c4b09532ce4504e2a655254f479c32abc 

Is there any value in that flag anymore? I can't see it used anywhere else in the code, and now that `load_table` is using `set_columns` it really needs an `init`'d table.

I've also formatted the C to make the CI happy in https://github.com/tskit-dev/tskit/pull/530/commits/8ae100ccb7e2b372b9c757090e5c597013032ded
comment
created
jeromekelleher
username_0
Awesome, thanks @username_1! I'll finish this up first thing tomorrow morning.
comment
created
jeromekelleher
username_0
Some more updates here which should push the test coverage in C right up, as well as put some infrastructure in place for making all the "x, x_offset" columns optional. I still need to add some tests for handling the input of bad offsets, but it'll be ready for final review and merge then.
comment
created
benjeffery
username_1
Great, I think making the "both columns" error explicit is a good choice. 

Speaking of coverage - is it worth getting the coverage to ignore some of the lines that we can't easily hit like OOMs? I'd like to enforce 100% coverage then.
comment
created
jeromekelleher
username_0
If there's an easy way to do this, then great. I'm wary about *enforcing* 100% test coverage as it leads to weird code and tests sometimes, but having more insight into the catchable errors would be good.

It is *possible* to test all the OOM corner cases (I've done it for [kastore](https://github.com/tskit-dev/kastore/blob/master/c/malloc_tests.c)) but it's pretty tedious.
comment
created
jeromekelleher
username_0
OK, I think this is ready to go. Can you have a look over please @username_1?

We probably won't be able to hit the test coverage threshold as there were a few incidental changes that have a lot of uncatchable errors, but I think we're covering the file format stuff pretty well now.

I hope you don't mind me squashing your "fixup!" commits, but I figured you wouldn't since they were marked as fixups.
comment
created
petrelharp
username_2
Fantastic! :yay:
comment