Skip to content

Allow skipping of sites and mutations in sort #1826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 22, 2021

Conversation

benjeffery
Copy link
Member

Fixes #1475

@benjeffery benjeffery force-pushed the sort-options branch 2 times, most recently from abbd341 to a1b5e03 Compare October 20, 2021 13:56
@benjeffery
Copy link
Member Author

Note that we could make the breaking change to make edge_start keyword only.

@codecov
Copy link

codecov bot commented Oct 20, 2021

Codecov Report

Merging #1826 (ce64c23) into main (cc9e445) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1826      +/-   ##
==========================================
- Coverage   93.43%   93.43%   -0.01%     
==========================================
  Files          27       27              
  Lines       24810    24808       -2     
  Branches     1094     1094              
==========================================
- Hits        23181    23179       -2     
  Misses       1594     1594              
  Partials       35       35              
Flag Coverage Δ
c-tests 92.20% <ø> (-0.01%) ⬇️
lwt-tests 89.14% <ø> (ø)
python-c-tests 94.54% <100.00%> (+<0.01%) ⬆️
python-tests 98.75% <100.00%> (ø)

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

Impacted Files Coverage Δ
c/tskit/tables.c 90.29% <ø> (-0.01%) ⬇️
python/_tskitmodule.c 91.69% <100.00%> (+<0.01%) ⬆️
python/tskit/tables.py 98.79% <100.00%> (ø)

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 cc9e445...ce64c23. Read the comment docs.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM. It's probably not worth making edge_start keyword only (could be unexpected breakage in downstream apps like tsinfer).

goto out;
}
memset(&start, 0, sizeof(start));
start.edges = edge_start;
start.sites = site_start;
Copy link
Member

Choose a reason for hiding this comment

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

Paranoia, but possibly safer the cast these to tsi_size_t

@@ -3124,6 +3124,10 @@ def sort(self, edge_start=0):
are not affected. This parameter is provided to allow for efficient sorting
when the user knows that the edges up to a given index are already sorted.

If both ``site_start`` and ``mutation_start`` are equal to the number of rows
in their retrospective tables then both are not sorted. Note that a partial
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "then neither is sorted"? Lot of negatives to wade through otherwise I think.

@benjeffery
Copy link
Member Author

All fixed up, merging.

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.

Sort only the edge table
2 participants