Individual parents #866

ivan-krukov · 2020-09-28T15:44:26Z

Implement a ragged parent column, as per #852

ivan-krukov · 2020-09-28T16:12:31Z

This is not looking good. Pulled from main and trying again

jeromekelleher · 2020-09-28T16:23:42Z

Yes, looks like it needs a rebase. This guide might help, if you're not familiar with rebasing.

jeromekelleher · 2020-09-28T16:24:49Z

Probably got hit by #862, which made a big change to the low-level module. I can jump in and bring things up to date if you're getting stuck.

ivan-krukov · 2020-09-28T16:51:15Z

Looks like I was able to resolve the merge conflict. The failing tests are checking the output file formats. Should be relatively easy to adjust that

codecov · 2020-09-28T18:01:18Z

Codecov Report

Merging #866 (d6efcb1) into main (e1eef38) will decrease coverage by 0.06%.
The diff coverage is 86.39%.

@@            Coverage Diff             @@
##             main     #866      +/-   ##
==========================================
- Coverage   93.70%   93.63%   -0.07%     
==========================================
  Files          26       26              
  Lines       21050    21204     +154     
  Branches      894      894              
==========================================
+ Hits        19724    19855     +131     
- Misses       1289     1312      +23     
  Partials       37       37

Flag	Coverage Δ
c-tests	`92.33% <75.26%> (-0.16%)`	⬇️
lwt-tests	`92.78% <92.00%> (-0.03%)`	⬇️
python-c-tests	`94.88% <100.00%> (+0.03%)`	⬆️
python-tests	`98.64% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
c/tskit/tables.c	`90.48% <75.26%> (-0.27%)`	⬇️
python/_tskitmodule.c	`91.37% <100.00%> (+0.06%)`	⬆️
python/lwt_interface/tskit_lwt_interface.h	`95.04% <100.00%> (+0.12%)`	⬆️
python/tskit/tables.py	`99.60% <100.00%> (+<0.01%)`	⬆️
python/tskit/trees.py	`97.47% <100.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 e1eef38...d6efcb1. Read the comment docs.

benjeffery · 2020-11-13T17:39:32Z

@ivan-krukov Just checking in as this PR has been inactive for a couple of months - are you planning to pick this up? Looks like it is close to done, I can finish it off if you're busy with other things?

ivan-krukov · 2021-01-14T16:12:23Z

@benjeffery Sorry, I was really MIA on this lately.
I am now looking into merging this again. Looks like there are a few conflicts, but they are minor.
Would you mind giving this a review when it is ready?

Thank you

jeromekelleher · 2021-01-14T17:00:30Z

Great, thanks @ivan-krukov! Would you mind bringing this up to date by rebasing please? If you get bogged down in git stuff Ben or I can help out.

…dual_parents

jeromekelleher

This looks great @ivan-krukov! I've had a medium scan of the changes, and I think you've basically covered everything. The rest is going to be tricky compatibility stuff that it's probably best if @benjeffery and I do.

So, how about we take this PR from here? If you could rebase and squash the commits please I think we can finish this up pretty quickly then.

jeromekelleher · 2021-01-18T13:16:06Z

python/tskit/tables.py

 class IndividualTableRow:
    flags: int
    location: np.ndarray
+    parents: np.array


jeromekelleher · 2021-01-18T13:18:25Z

python/tskit/tables.py

        return headers, rows

-    def add_row(self, flags=0, location=None, metadata=None):
+    def add_row(self, flags=0, location=None, parents=None, metadata=None):


For strict compatibility we should put the parents at the of the args list. I'm tempted to make everything after flags a keyword-only argument though. @benjeffery, any thoughts?

Same point applies in a few other places where we're updating the public API.

A few months back we considered reviewing the API to make more args keyword-only. I think it's a good idea.

I also think this is a good idea. Much of this commit is just duplicating the location table code. I feel like a lot of this can be streamlined.

jeromekelleher · 2021-01-18T13:21:05Z

c/tskit/tables.c

        goto out;
    }
+    if (parents_offset[num_rows] != parents_length) {
+        ret = TSK_ERR_BAD_OFFSET;


We should try to cover this line. @benjeffery, where do we catch this sort of stuff again?

This sort of thing should be hit by adding the column to the list of ragged columns in test_tables.py:845. I'm not sure why it isn't, but could look into it.

I've added that at test_tables.py:847, in addition to some basic tests below. I was also confused why the codecov/patch did not pass.

ivan-krukov · 2021-01-18T16:05:09Z

@jeromekelleher , @benjeffery Please ignore the last two commits then. I will make the corrections above and squash.

ivan-krukov · 2021-01-18T18:00:41Z

@benjeffery @jeromekelleher I am embarrassed to admit that I am having trouble with squashing the commits. It seems that since it has diverged so long ago, I am getting more merge conflicts.

Could you please merge this for me? The #individual_parents_squash_me is up-to-date with the changes

https://github.com/ivan-krukov/tskit/tree/individual_parent_squash_me

Thank you

jeromekelleher · 2021-01-19T07:44:57Z

I had a go at rebasing this @ivan-krukov, but it's a bit of a mess unfortunately because it cuts across some large code changes that happened since (mainly, the "constification" of the C API and also pulling out the dict interface in the Python C layer into the "lwt" file). I think it's best to go down the when rebasing goes wrong path.

I'd suggest doing the following:

Apply your diff to a clean branch. Focus on the C library first - get that cleaned up, compiling and passing tests. Add just those changes in one commit.
Next look at the _msprimemodule.c and lwt_interface.c. The diff will be messy here, but it should be possible to identify your bits that should go in either file. Get those compiling and passing the test_lowlevel.py tests. Commit those changes.
Finally bring the Python code up to date, and commit.

Once that's done, open a new PR, which we (the maintainers) can then pick up and complete.

How does that sound?

ivan-krukov · 2021-01-22T02:22:12Z

Closing this in favour of #1125

ivan-krukov added 5 commits September 27, 2020 14:43

Adding parent column - initial changes with dummy tests

2499d49

Tests for parsing parent column

0476802

Table output

1ac02b1

Added bindings to the python module

3685368

Merge branch 'main' of github.com:tskit-dev/tskit into main

c691e8d

ivan-krukov added 2 commits September 28, 2020 12:16

Merged with the lwt changes

0f5af71

Constructor arguments, formatting

725e2dd

Clang-format and black

56e27ad

Fixed failing tests

40a90c0

ivan-krukov added 5 commits January 17, 2021 12:42

Merge branch 'main' of https://github.com/tskit-dev/tskit into indivi…

b689e3c

…dual_parents

Code formatiing

54ec7f6

Manual merge of lwt_interface

bf64be3

Fixing loose ends

42f3fb4

Formatting C-python glue code

c9dae5f

jeromekelleher reviewed Jan 18, 2021

View reviewed changes

benjeffery mentioned this pull request Jan 18, 2021

Add KEEP_UNARY_IN_INDIVIDUALS option to simplify() #1119

Merged

3 tasks

Type correction

cdf8752

Resolve review comment

d6efcb1

ivan-krukov mentioned this pull request Jan 22, 2021

Individual parents merge #1125

Merged

3 tasks

ivan-krukov closed this Jan 22, 2021

Individual parents #866

Individual parents #866

Uh oh!

Conversation

ivan-krukov commented Sep 28, 2020

Uh oh!

ivan-krukov commented Sep 28, 2020

Uh oh!

jeromekelleher commented Sep 28, 2020

Uh oh!

jeromekelleher commented Sep 28, 2020

Uh oh!

ivan-krukov commented Sep 28, 2020

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

benjeffery commented Nov 13, 2020

Uh oh!

ivan-krukov commented Jan 14, 2021

Uh oh!

jeromekelleher commented Jan 14, 2021

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!

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!

ivan-krukov commented Jan 18, 2021

Uh oh!

ivan-krukov commented Jan 18, 2021

Uh oh!

jeromekelleher commented Jan 19, 2021

Uh oh!

ivan-krukov commented Jan 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 28, 2020 •

edited

Loading