Add index to the LWT representation #921

benjeffery · 2020-10-22T13:41:41Z

Description

Adds indexes (optionally) to the LWT interface. Based on #916 as it needs python access to indexes.
Fixes #870

PR Checklist:

Tests that fully cover new/changed functionality.
Documentation including tutorial content if appropriate.
Changelogs, if there are API changes.

AdminBot-tskit · 2020-10-22T13:43:16Z

📖 Docs for this PR can be previewed here

benjeffery · 2020-10-22T14:25:25Z

I recommend merging #916 first then asking mergify to rebase before review.

codecov · 2020-10-22T14:49:56Z

Codecov Report

Merging #921 into main will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #921      +/-   ##
==========================================
- Coverage   93.40%   93.40%   -0.01%     
==========================================
  Files          25       25              
  Lines       20335    20357      +22     
  Branches      824      825       +1     
==========================================
+ Hits        18994    19014      +20     
- Misses       1304     1306       +2     
  Partials       37       37

Impacted Files	Coverage Δ
lwt_interface/tskit_lwt_interface.h	`94.79% <0.00%> (-0.14%)`	⬇️
_tskitmodule.c	`90.21% <0.00%> (+<0.01%)`	⬆️
tskit/tables.py	`99.71% <0.00%> (+<0.01%)`	⬆️

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 5626ce8...89295ea. Read the comment docs.

jeromekelleher

LGTM, minor comments.

jeromekelleher · 2020-10-23T13:33:31Z

c/tskit/tables.h

+@endrst
+
+@param self A pointer to a tsk_table_collection_t object.
+@param edge_insertion_order Array of tsk_id_t edge ids


Full stop missing here and next

jeromekelleher · 2020-10-23T13:37:46Z

python/lwt_interface/tskit_lwt_interface.h

    }

+    /* index */
+    value = get_table_dict_value(tables_dict, "index", false);


Probably best to call this "indexes" just for consistency with the C struct?

For consistency we should also change the python property on TableCollection to indexes but then that is inconsistent with build_index and has_index which would be breaking changes to move to indexes... we may have to live with the python API being index and the c struct being indexes.

jeromekelleher · 2020-10-23T14:05:00Z

Need to do some tests where we add bad indexes from the dict encoding. Things like:

Negative/out of bounds edge IDs
Nonsensical indexes (np.zeros for both, e.g.). What error occurs here when we try to run ts.trees()?

benjeffery · 2020-10-26T14:34:41Z

Need to do some tests where we add bad indexes from the dict encoding. Things like:

Negative/out of bounds edge IDs

Nonsensical indexes (np.zeros for both, e.g.). What error occurs here when we try to run ts.trees()?

As it was nothing happened as indexes were always rebuilt on load_tables I'm adding a build_indexes flag to the python API in #916

benjeffery · 2020-10-27T11:40:59Z

Almost there, needs a few more tests so setting to draft.

benjeffery · 2020-10-27T14:01:06Z

@jeromekelleher I think this is ready to go - I've changed the sematics such that TableCollections without indexes now give None instead of a dict with empty arrays. Previously roundtripping a TableCollection without indexes resulted in adding zero-length indexes, which didn't seem right!

jeromekelleher

I'm not sure None is the right encoding for missing indexes here - see comment above.

jeromekelleher · 2020-10-27T14:59:20Z

python/lwt_interface/CHANGELOG.rst

+[0.1.2] - 2020-10-22
+--------------------
+
+ - Added optional top-level key ``index`` which has contains ``edge_insertion_order`` and


Fixed in 9618a3b

jeromekelleher · 2020-10-27T15:05:48Z

python/_tskitmodule.c

        goto out;
    }

+    if (!tsk_table_collection_has_index(self->tables, 0)) {


I don't know about this. I think it would be logical to return an empty dictionary. That way we're more robust to adding more indexes in the future, where the dictionary may contain the current edge_x indexes, but also maybe any future indexes. So, we might have to deal with the situation where the dictionary contains a variable number of keys - logically then, it makes sense to treat the zero-keys situation in the same way.

Think of tsk_table_collection_has_index(self, 0) as saying "do you have all possible indexes". So, we'd have (roughly)

dict = PyDict_New(); if (tsk_table_collection_has_index(self->tables, 0)) { // add key-value pairs for each edge_insertion and edge_removal }

We can see how this is then extensible to having more indexes in the future - we can just test the existance of the each index on a case-by-case basis.

Fixed in 2855dec

jeromekelleher

Not sure the lwt encoding version is the same as the output of asdict() now? We should probably add a test that these are identical, somewhere.

jeromekelleher · 2020-10-27T17:17:15Z

python/lwt_interface/tskit_lwt_interface.h

    };

    for (j = 0; j < sizeof(table_descs) / sizeof(*table_descs); j++) {
+        if (strcmp(table_descs[j].name, "indexes") == 0


Do we need to do this? I think it'd be better if the dict encoding had the empty dictionary as the output rather than excluding the key. This is what the TableCollection does, right?

Fixed in 4d81529

benjeffery · 2020-11-02T14:57:26Z

@jeromekelleher I've tidied this up and added the test for identical contents in the dicts.

jeromekelleher · 2020-11-02T17:59:51Z

LGTM, merge away whenever

Add index to the LWT representation

benjeffery force-pushed the lwt-index branch 2 times, most recently from bf2772d to d13efbe Compare October 22, 2020 14:22

benjeffery changed the title ~~[WIP] Lwt index~~ Add index to the LWT representation Oct 22, 2020

benjeffery requested a review from jeromekelleher October 22, 2020 14:22

benjeffery marked this pull request as ready for review October 22, 2020 14:22

jeromekelleher reviewed Oct 23, 2020

View reviewed changes

benjeffery mentioned this pull request Oct 26, 2020

Python access to Table Collection index #916

Merged

benjeffery force-pushed the lwt-index branch 3 times, most recently from 6861686 to 89f158c Compare October 27, 2020 11:40

benjeffery marked this pull request as draft October 27, 2020 11:40

benjeffery force-pushed the lwt-index branch 2 times, most recently from ae950d1 to c8d6d44 Compare October 27, 2020 13:55

benjeffery marked this pull request as ready for review October 27, 2020 13:59

benjeffery force-pushed the lwt-index branch from c8d6d44 to f1f0087 Compare October 27, 2020 14:17

jeromekelleher reviewed Oct 27, 2020

View reviewed changes

benjeffery force-pushed the lwt-index branch from 1893898 to 2855dec Compare October 27, 2020 15:48

jeromekelleher reviewed Oct 27, 2020

View reviewed changes

benjeffery force-pushed the lwt-index branch 3 times, most recently from c3a2feb to 4d81529 Compare November 2, 2020 14:54

jeromekelleher approved these changes Nov 2, 2020

View reviewed changes

benjeffery added the AUTOMERGE-REQUESTED label Nov 2, 2020

benjeffery added 4 commits November 2, 2020 23:30

Add table collection index to LWT interface

480aaa3

pluralise

bc81e63

Return empty dict instead of None

20575a9

Add test for concordence with lwt

89295ea

AdminBot-tskit force-pushed the lwt-index branch from 4d81529 to 89295ea Compare November 2, 2020 23:30

mergify bot merged commit e40dff7 into tskit-dev:main Nov 2, 2020

mergify bot removed the AUTOMERGE-REQUESTED label Nov 2, 2020

benjeffery pushed a commit that referenced this pull request Nov 2, 2020

Merge pull request #921 from benjeffery/lwt-index

bd5eb19

Add index to the LWT representation

benjeffery deleted the lwt-index branch November 10, 2020 22:32

Add index to the LWT representation #921

Add index to the LWT representation #921

Uh oh!

Conversation

benjeffery commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR Checklist:

Uh oh!

AdminBot-tskit commented Oct 22, 2020

Uh oh!

benjeffery commented Oct 22, 2020

Uh oh!

codecov bot commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeromekelleher commented Oct 23, 2020

Uh oh!

benjeffery commented Oct 26, 2020

Uh oh!

benjeffery commented Oct 27, 2020

Uh oh!

benjeffery commented Oct 27, 2020

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjeffery commented Nov 2, 2020

Uh oh!

jeromekelleher commented Nov 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benjeffery commented Oct 22, 2020 •

edited

Loading

codecov bot commented Oct 22, 2020 •

edited

Loading