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

GroupReadsByUmi only sort input if it is not TemplateCoordinate sorted #794

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

nh13
Copy link
Member

@nh13 nh13 commented Feb 27, 2022

This extracts some functionality from #791, where we skip sorting the input if the input is already TemplateCoordinate sorted. This allows other tools to perform the sort.

@nh13 nh13 requested a review from tfenne February 27, 2022 22:42
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2022

Codecov Report

Merging #794 (86e8980) into main (6e0a53d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #794      +/-   ##
==========================================
+ Coverage   95.51%   95.53%   +0.01%     
==========================================
  Files         122      122              
  Lines        7063     7071       +8     
  Branches      500      505       +5     
==========================================
+ Hits         6746     6755       +9     
+ Misses        317      316       -1     
Flag Coverage Δ
unittests 95.53% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...cala/com/fulcrumgenomics/umi/GroupReadsByUmi.scala 95.35% <100.00%> (+0.16%) ⬆️
src/main/scala/com/fulcrumgenomics/bam/Bams.scala 95.72% <0.00%> (+0.85%) ⬆️

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 6e0a53d...86e8980. Read the comment docs.

@nh13 nh13 added the 2.0 label Feb 28, 2022
Copy link
Member

@tfenne tfenne left a comment

Choose a reason for hiding this comment

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

canTakeNextGroupByUmi on line 480 will, I think, need updating. If the sort was done externally then I don't think we can rely on the fact that they records are also sorted by the same UMI sequence we're going to look at, and so it's not safe to take the next group by UMI.

Usage needs updating to clearly explain when sorting will and won't happen.

@tfenne tfenne assigned nh13 and unassigned tfenne Mar 1, 2022
@nh13 nh13 assigned tfenne and unassigned nh13 Mar 2, 2022
Copy link
Member

@tfenne tfenne left a comment

Choose a reason for hiding this comment

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

Looks good except for updating usage and fixing up the canTakeNextGroupByUmi.

@tfenne tfenne assigned nh13 and unassigned tfenne Mar 2, 2022
@nh13 nh13 assigned tfenne and unassigned nh13 Mar 3, 2022
@nh13
Copy link
Member Author

nh13 commented Mar 3, 2022

Done.

@nh13 nh13 force-pushed the nh-group-reads-skip-sorting branch from 33ff57b to 86e8980 Compare March 3, 2022 03:35
@tfenne tfenne self-requested a review March 4, 2022 17:34
@tfenne tfenne assigned nh13 and unassigned tfenne Mar 10, 2022
@nh13 nh13 merged commit a100106 into main Mar 10, 2022
@nh13 nh13 deleted the nh-group-reads-skip-sorting branch March 10, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants