Skip to content
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

Don't sort individuals by default #1789

Merged
merged 1 commit into from
Oct 20, 2021

Conversation

benjeffery
Copy link
Member

@benjeffery benjeffery commented Oct 13, 2021

Part of #1774

I started ripping loads of code out, then realised that maybe this is a less destructive way to do this. (Commented out code will return when the individual sort method uses it.)

WIP for feedback, next is removing the requirement from checks

@jeromekelleher
Copy link
Member

This is tricky - let's have a chat about it tomorrow, see if we can figure out a clean path forward?

@benjeffery benjeffery force-pushed the skip-individual-sort branch 2 times, most recently from 97911c2 to 7a2e265 Compare October 18, 2021 16:33
@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #1789 (b2e57c5) into main (6142f7f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head b2e57c5 differs from pull request most recent head 9f01305. Consider uploading reports for the commit 9f01305 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1789      +/-   ##
==========================================
+ Coverage   93.41%   93.43%   +0.01%     
==========================================
  Files          27       27              
  Lines       24756    24716      -40     
  Branches     1095     1095              
==========================================
- Hits        23127    23093      -34     
+ Misses       1594     1588       -6     
  Partials       35       35              
Flag Coverage Δ
c-tests 92.18% <100.00%> (+0.02%) ⬆️
lwt-tests 89.14% <ø> (ø)
python-c-tests 94.55% <ø> (ø)
python-tests 98.75% <ø> (ø)

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

Impacted Files Coverage Δ
c/tskit/tables.c 90.23% <100.00%> (+0.03%) ⬆️

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 6142f7f...9f01305. Read the comment docs.

@benjeffery benjeffery force-pushed the skip-individual-sort branch 3 times, most recently from 9f01305 to 1fbb102 Compare October 19, 2021 13:17
@benjeffery benjeffery marked this pull request as ready for review October 19, 2021 13:19
@benjeffery
Copy link
Member Author

@jeromekelleher I think this is ready, although do we need to check integrity before we sort?

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, some minor tweaks.

Nice minimally destructive approach!

c/tskit/tables.h Outdated
@return Return 0 on success or a negative value on failure.
*/
int tsk_table_collection_sort(
tsk_table_collection_t *self, const tsk_bookmark_t *start, tsk_flags_t options);

/**
@brief Sorts the indivdual table in this collection.
Copy link
Member

Choose a reason for hiding this comment

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

typo, indivdual

@@ -3165,6 +3164,15 @@ def sort(self, edge_start=0):
self._ll_tables.sort(edge_start)
# TODO add provenance

def sort_individuals(self, edge_start=0):
Copy link
Member

Choose a reason for hiding this comment

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

Remove edge_start param

c/tskit/tables.c Outdated
@@ -6379,7 +6380,7 @@ tsk_table_sorter_run(tsk_table_sorter_t *self, const tsk_bookmark_t *start)
goto out;
}
}
if (!skip_individuals) {
if (!skip_individuals && self->sort_individuals) {
Copy link
Member

Choose a reason for hiding this comment

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

Comparisons to NULL are usually explicit, i.e., && (self->sort_individuals != NULL)

Reading this I assumed that self->sort_individuals is a boolean

@jeromekelleher
Copy link
Member

We should update the CHANGELOGs in case we forget.

@benjeffery
Copy link
Member Author

Nice minimally destructive approach!

4th times the charm....

Did you see the question about checking integrity before sort, I think as it stands you could make python segfault with this.

@jeromekelleher
Copy link
Member

I think as it stands you could make python segfault with this.

I haven't thought about it, but the answer is in the question if this is true!

@benjeffery benjeffery force-pushed the skip-individual-sort branch 2 times, most recently from bc0bc4f to c5802b4 Compare October 20, 2021 00:51
@benjeffery benjeffery force-pushed the skip-individual-sort branch from c5802b4 to 3a88790 Compare October 20, 2021 09:25
@benjeffery
Copy link
Member Author

@jerome all fixed up and changlog added, assuming CI is now happy this is good to go.

@jeromekelleher jeromekelleher added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Oct 20, 2021
@mergify mergify bot merged commit e622480 into tskit-dev:main Oct 20, 2021
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Oct 20, 2021
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.

2 participants