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

filter_nodes for simplify #2606

Closed

Conversation

jeromekelleher
Copy link
Member

Here's a quick first pass for #2605.

I think it basically works, but would need more testing and a bit of implementation polish (mainly just properly working out whether a node is a sample).

The main limitation is that the input samples must be the original tree sequence samples. We could probably relax that a bit, I guess (make it a subset?) but I didn't think it through.

@hyanwong - do you want to try this out on your applications and maybe come up with some tests?

@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #2606 (5bcbc30) into main (5a3ebde) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2606   +/-   ##
=======================================
  Coverage   93.91%   93.91%           
=======================================
  Files          27       27           
  Lines       27200    27200           
  Branches     1215     1215           
=======================================
  Hits        25544    25544           
  Misses       1622     1622           
  Partials       34       34           
Flag Coverage Δ
c-tests 92.24% <ø> (ø)
lwt-tests 89.05% <ø> (ø)
python-c-tests 73.14% <ø> (ø)
python-tests 98.95% <ø> (ø)

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


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 5a3ebde...5bcbc30. Read the comment docs.

@hyanwong
Copy link
Member

I think we decided that we should allow any set of nodes to be specified as samples, although I'd be happy simply to restrict it to ts.samples(), at least for the time being.

If we did allow different sample sets with filter_nodes=False, I think we said that the order of samples passed to the function should not be important (so that nodes never change their ID). Alternatively, we could simply error out if the node IDs provided as samples are not in ascending order.

@hyanwong hyanwong mentioned this pull request Oct 30, 2022
3 tasks
@hyanwong
Copy link
Member

I wasn't sure how to add extra functionality to this PR (or whether I have permission to), so I have opened up another PR with the commit in this PR as the first commit and a few tests added as extra commits: #2619. Hence I'm closing this

@hyanwong hyanwong closed this Oct 30, 2022
@jeromekelleher jeromekelleher deleted the simplify-filter-nodes branch October 31, 2022 09:31
@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2022

⚠️ The sha of the head commit of this PR conflicts with #2619. Mergify cannot evaluate rules on this PR. ⚠️

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.

None yet

2 participants