Add TSK_NO_EDGE_METADATA option, which disables the metadata column on edges #712
Conversation
Codecov Report
@@ Coverage Diff @@
## master #712 +/- ##
===========================================
+ Coverage 87.46% 98.93% +11.47%
===========================================
Files 24 15 -9
Lines 18162 4240 -13922
Branches 3609 750 -2859
===========================================
- Hits 15886 4195 -11691
+ Misses 1099 17 -1082
+ Partials 1177 28 -1149
Continue to review full report at Codecov.
|
jeromekelleher
left a comment
jeromekelleher
left a comment
There was a problem hiding this comment.
LGTM so far, some minor comments.
c/tests/test_tables.c
Outdated
| ret = tsk_table_collection_init(&tables, TSK_NO_EDGE_METADATA); | ||
| CU_ASSERT_EQUAL_FATAL(ret, 0); | ||
| tables.sequence_length = 10; | ||
| tsk_edge_table_add_row(&tables.edges, 0, 0, 1, 1, "metadata", 8); |
There was a problem hiding this comment.
We're missing a ret check here, aren't we? This call should fail, right?
c/tskit/tables.c
Outdated
| (void **) &self->metadata_offset, new_size + 1, sizeof(tsk_size_t)); | ||
| if (ret != 0) { | ||
| goto out; | ||
| if (!(self->options & TSK_NO_METADATA)) { |
There was a problem hiding this comment.
we'll probably be doing this a lot, so maybe worth a static function, tsk_edge_table_has_metadata(self)?
c/tskit/tables.c
Outdated
|
|
||
| if (!(options & TSK_NO_INIT)) { | ||
| ret = tsk_edge_table_init(dest, 0); | ||
| ret = tsk_edge_table_init(dest, self->options); |
There was a problem hiding this comment.
It's not obvious to me we want to do this. Perhaps the table options should from the options parameter?
There was a problem hiding this comment.
I did this to avoid the case of turning metadata back on for a table that was init'd with it off. I've added an error for that case so that now you can disable metadata on a copy. 071d93b
c/tskit/tables.c
Outdated
| self->metadata + self->metadata_offset[j]); | ||
| if (err < 0) { | ||
| goto out; | ||
| if (self->options & TSK_NO_METADATA) { |
There was a problem hiding this comment.
I think it'd be better if we use ``edges_table_get_row here and print out the properties of the returned edge object (we can ignore the return value). That way we can just print the values.
|
@jeromekelleher I've addressed comments and implemented the change so that metadata is re-instated when tables are used as a tree sequence. Unless you can think of tests I've missed this is good to go. |
|
Thanks @benjeffery. Sorry, i don't have time to go through it now. Is it OK if I go through an make any final tweaks next week and merge? (Maybe you could squash now?) |
benjeffery
force-pushed
the
no-edge-metadata
branch
from
324651c to
d51c767
Compare
|
I've squashed - go ahead with any changes you need to any branch while I'm off. |
|
Looks great, thanks again @benjeffery! I made a couple of minor tweaks. Mainly to make absolutely sure that equals in EdgeTable is safe. |
jeromekelleher
force-pushed
the
no-edge-metadata
branch
from
fc2e4dc to
c8a54d3
Compare
2 participants
Closes #585
I've got far enough with this that feedback would be useful. The big decision is to add
optionstotsk_edge_table_t. This seems cleaner and more preferable to usingmetadata==NULLto tell if the table has metadata.Currently when an edge table with
TSK_NO_METADATAis dumped the metadata columns are absent from the kastore. This is fine as those are optional. When the table is re-loaded, however metadata will be re-enabled. If we storedoptionsin the kastore we could load a table with metadata disabled.I've also opted to error if you try to set metadata on the table.
There are probably a few more tests needed. I've started work on #708, but in doing that realised it would be nice to have this done first.