Skip to content

canonical sorting #715

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

Closed
wants to merge 4 commits into from
Closed

canonical sorting #715

wants to merge 4 commits into from

Conversation

mufernando
Copy link
Member

Currently, sort does not put a Table Collection in canonical form (given the order of nodes) -- see #705 .

Here I am working on adding mutation time, mutation parent and node to the mutation sorting key.

(still very early, working on a python impl of sort first!)

@jeromekelleher
Copy link
Member

Looks great @mufernando, this is exatctly the right approach. Ping me when you'd like some feedback.

@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #715 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #715   +/-   ##
=======================================
  Coverage   87.46%   87.46%           
=======================================
  Files          24       24           
  Lines       18162    18162           
  Branches     3609     3609           
=======================================
  Hits        15886    15886           
  Misses       1099     1099           
  Partials     1177     1177           
Flag Coverage Δ
#c_tests 89.00% <ø> (ø)
#python_c_tests 90.61% <ø> (ø)
#python_tests 99.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 9e96bf4...c543d3e. Read the comment docs.

@jeromekelleher
Copy link
Member

@mufernando - is this something that we want to get in for the C API release, or will having it done for the next Python release be enough? We'd really like to get the C API shipped soon, and I think just getting the sorting requirement done will be enough for that. Is that OK, or do you want the canonicalise (or whatever) method to do into the C API in time for the next release too? (The only practical reason for this is if you want to use it in SLiM, I think)

@mufernando
Copy link
Member Author

I don't need this in SLiM and probably won't be able to get to this over the next week or two, so we can wait for the next release.

@benjeffery benjeffery changed the base branch from master to main September 28, 2020 12:12
@benjeffery
Copy link
Member

@mufernando Just pinging as this PR has been inactive for a while - do you plan to pick it up?

@mufernando
Copy link
Member Author

I don't think I'll have the time to work on this for a while. But it seems the work I did with validating the C sort with a Python implementation could be incorporated. Is it bad to leave this hanging here?

@benjeffery
Copy link
Member

It's no problem to leave it - just useful for planning releases to know which PRs are still active.

@benjeffery
Copy link
Member

Superseded by #1108

@benjeffery benjeffery closed this Mar 16, 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.

4 participants