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

First pass at trim functions #292

Closed
wants to merge 1 commit into from
Closed

Conversation

hyanwong
Copy link
Member

@hyanwong hyanwong commented Aug 8, 2019

Closes #288

No tests yet, as these will be easier once #261 is committed, so that we can easily make tree sequences with no trees to left or right.

Also the documentation will need improving (e.g. document the TableCollection methods). I will do this once the general approach has been approved by @jeromekelleher .

@hyanwong
Copy link
Member Author

hyanwong commented Aug 8, 2019

Once #261 is in, it would be rather easy to leave slice in as a convenience function. The documentation could simply say something like "ts.slice(start, stop): a convenient wrapper for ts.keep_interval([(start, stop)]).trim().

But I'm equally happy not to have this.

@hyanwong
Copy link
Member Author

hyanwong commented Aug 9, 2019

The code in remove_sites duplicates code in the proposed keep_intervals() function in #261. But I think we do need to remove sites in trim, as we don't know beforehand if the TS to trim has come from keep_intervals or not. If not, we might end up with negative positions for some sites.

I suspect the remove_sites functionality is more generally useful too. I've coded it up here as a tables function, so it should be possible (although maybe less efficient) to use it in keep_intervals, if we want to reduce code duplication.

@hyanwong hyanwong force-pushed the trim branch 4 times, most recently from 4856992 to 2dd55ac Compare August 15, 2019 11:53
@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #292 into master will decrease coverage by 0.3%.
The diff coverage is 29.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
- Coverage   86.25%   85.95%   -0.31%     
==========================================
  Files          20       20              
  Lines       14086    14146      +60     
  Branches     2749     2755       +6     
==========================================
+ Hits        12150    12159       +9     
- Misses       1010     1061      +51     
  Partials      926      926
Flag Coverage Δ
#c_tests 86.82% <29.16%> (-0.42%) ⬇️
#python_c_tests 89.64% <29.16%> (-0.67%) ⬇️
#python_tests 97.5% <29.16%> (-1.71%) ⬇️
Impacted Files Coverage Δ
python/tskit/util.py 100% <100%> (ø) ⬆️
python/tskit/tables.py 93.9% <19.51%> (-5.91%) ⬇️
python/tskit/trees.py 96.94% <35.71%> (-1.59%) ⬇️

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 eb3847f...a0dc3fb. Read the comment docs.

@jeromekelleher
Copy link
Member

This is quite a lot of code and will need a lot of testing. Can we push this back to after 0.2.0?

@hyanwong
Copy link
Member Author

Yes - I'd much prefer to prioritise getting 0.2.0 out. I've only used the trim functionality (which was in slice()) in one bit of non-reusable code anyway, so it's probably just me requiring this at the moment.

@hyanwong
Copy link
Member Author

Closing with the intent of opening a new PR, since the remove_sites functionality is now coded elsewhere

@hyanwong hyanwong closed this Sep 26, 2019
@hyanwong hyanwong mentioned this pull request Sep 27, 2019
@hyanwong hyanwong deleted the trim branch August 25, 2020 16:33
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.

Implement ltrim(), rtrim(), and trim() to remove genomic areas with no edges
2 participants