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

adding new ReadQueryNameComparator #4731

Merged
merged 1 commit into from May 3, 2018

Conversation

lbergelson
Copy link
Member

  • this should match the results of SAMRecordQueryNameComparator exactly
  • it operates on GATKRead instead of SAMRecord

* this should match the results of SAMRecordQueryNameComparator exactly
* it operates on GATKRead instead of SAMRecord
Copy link
Collaborator

@jamesemery jamesemery 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, its gross we have to copy so much code but there is no way to avoid this without converting to a SAMRecord which seems worse. 👍

}

@Test(dataProvider = "getReads")
public void testTieBreakers(GATKRead left, GATKRead right){
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks like a much cleaner approach than the equivalent approach in ReadsCoordinateComparator

@codecov-io
Copy link

Codecov Report

Merging #4731 into master will increase coverage by 0.015%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #4731       +/-   ##
===============================================
+ Coverage     79.977%   79.992%   +0.015%     
- Complexity     17397     17421       +24     
===============================================
  Files           1080      1081        +1     
  Lines          63093     63119       +26     
  Branches       10179     10195       +16     
===============================================
+ Hits           50460     50490       +30     
+ Misses          8648      8644        -4     
  Partials        3985      3985
Impacted Files Coverage Δ Complexity Δ
...hellbender/utils/read/ReadQueryNameComparator.java 100% <100%> (ø) 22 <22> (?)
...utils/smithwaterman/SmithWatermanIntelAligner.java 90% <0%> (+40%) 3% <0%> (+2%) ⬆️


/**
* compare {@link GATKRead} by queryname
* duplicates the exact ordering of {@link SAMRecordQueryNameComparator}
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplication of code from HTSJDK to GATK (and other downstream projects) is a sign that htsjdk needs to evolve to an interface-based - as @lbergelson is a maintainer, I point out again that #4340 and htsjdk-related issues should be a high priority!

Copy link
Member Author

Choose a reason for hiding this comment

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

@magicDGS You're very right. We had hoped to begin work on a major htsjdk revamp last quarter, but we didn't have enough bandwidth to take it on along with other requirements we had. We're hoping to start very soon.

@lbergelson lbergelson merged commit ac34d0b into master May 3, 2018
@lbergelson lbergelson deleted the lb_add_ReadQueryNameComparator branch May 3, 2018 16:43
cwhelan pushed a commit to cwhelan/gatk-linked-reads that referenced this pull request May 25, 2018
* added a new queryname comparator for GATKRead it operates on GATKRead instead of SAMRecord
* this matches the results of SAMRecordQueryNameComparator exactly
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

4 participants