Skip to content

Conversation

@jeromekelleher
Copy link
Member

Closes #2276

Taking over part of #2240

@jeromekelleher jeromekelleher force-pushed the split_edges branch 2 times, most recently from a3941ff to fca1d31 Compare May 24, 2022 12:28
@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #2296 (6d12abb) into main (76b6581) will decrease coverage by 0.00%.
The diff coverage is 88.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2296      +/-   ##
==========================================
- Coverage   93.33%   93.32%   -0.01%     
==========================================
  Files          27       27              
  Lines       26141    26256     +115     
  Branches     1175     1177       +2     
==========================================
+ Hits        24398    24503     +105     
- Misses       1713     1723      +10     
  Partials       30       30              
Flag Coverage Δ
c-tests 92.29% <85.50%> (-0.02%) ⬇️
lwt-tests 89.05% <ø> (ø)
python-c-tests 71.87% <70.21%> (-0.01%) ⬇️
python-tests 98.88% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
c/tskit/trees.c 95.02% <85.50%> (-0.22%) ⬇️
python/_tskitmodule.c 90.90% <91.42%> (+<0.01%) ⬆️
python/tskit/trees.py 98.06% <100.00%> (+0.01%) ⬆️
c/tskit/tables.c 90.33% <0.00%> (+0.04%) ⬆️

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 76b6581...6d12abb. Read the comment docs.

Copy link
Contributor

@petrelharp petrelharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very well tested! All good.

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, nice stuff.
Just a couple of comments:

  • It might be good to test what happens when split is called with t=unknown_time?
  • I can't see where the provenance info mentioned in the docstring is recorded?

if metadata is None:
metadata = tables.nodes.metadata_schema.empty_value
metadata = tables.nodes.metadata_schema.validate_and_encode_row(metadata)
# This is the easiest way to turn off encoding when calling add_row below
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this pattern here imply we should have a way to pass in encoded metadata to add_row? Maybe encoded_metadata=?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so - either that or have a boolean flag for controlling whether we encode or not?

@jeromekelleher
Copy link
Member Author

Thanks for the careful review @benjeffery, super helpful.

I think the simplest thing is to error with nonfinite time, and have added tests.

I can't be bothered with adding provenance, it feels like a waste of effort. It's easy enough to add later if we need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add TableCollection.split_edges

3 participants