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

Optimize intervals fails to merge abutting intervals #979

Conversation

Kirvolque
Copy link
Contributor

@Kirvolque Kirvolque commented Sep 11, 2017

Description

Optimizing intervals in QueryInterval class works incorrectly: intervals are considered abutting only if they overlap each other by a single base.

Prerequisites

Need bug fix for current implementation of abuts(): QueryInterval[] array still contains two initial intervals after invoking optimizeIntervals() method, even though they touch. Github issue.

Solution

  • abuts() method has been fixed. It used to return true only in case if one of them starts exactly where other ends. Now it returns true if one of the given intervals ends just before another one starts;
  • equals() and hashcode() methods were overridden in QueryInterval class for using assert in TestNg;
  • testOptimizeIntervals() was added to the CRAMFileBAIIndexTest class to test 3 options: abutting, overlapping and non-overlapping intervals.

Checklist

  • Code compiles correctly
  • New tests covering changes and new functionality
  • All tests passing
  • Extended the README / documentation, if necessary
  • Is not backward compatible (breaks binary or source compatibility)

@@ -42,10 +42,10 @@ public int compareTo(final QueryInterval other) {
}

/**
* @return true if both are on same reference, and other starts exactly where this ends.
* @return true if both are on same reference, and other starts exactly before this ends.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be changing the definition of "abuts". since we use closed intervals the implementation was correct before the change.

Copy link
Member

Choose a reason for hiding this comment

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

@yfarjoun If QueryInterval really uses closed intervals, then I think @Kirvolque is right and the existing implementation was wrong. E.g. two closed intervals 1-9 and 10-20 abut one another, but the coordinates are indeed off by one.

That said, I think this method should be private since it doesn't test bi-directionally. That's fine given how it's used in optimizeOverlaps, but it would be confusing if called by other code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I see. Sorry to judge so quick. The method is called from BAMFileReader, so it can't be changed to private. perhaps we just change it's name to something like endAtStartOf so that the code would read prev.endsAtStartOf(nextInterval).

Copy link
Contributor Author

@Kirvolque Kirvolque Sep 12, 2017

Choose a reason for hiding this comment

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

@yfarjoun, thank you. The method has been renamed.

@Kirvolque Kirvolque changed the title fix: Optimize intervals falls to merge abutting intervals Optimize intervals falls to merge abutting intervals Sep 11, 2017
@Kirvolque
Copy link
Contributor Author

@yfarjoun, thank you for quick response. If offered changes are unnecessary, may we close this pull request?

@@ -94,4 +94,24 @@ public String toString() {

return unique.toArray(EMPTY_QUERY_INTERVAL_ARRAY);
}

@Override
public boolean equals(Object o) {
Copy link
Member

Choose a reason for hiding this comment

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

In classes that implement Comparable it's preferable to implement equals() as this.compareTo(that) == 0 after the initial type checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tfenne, done. Thank you for your note.

@yfarjoun
Copy link
Contributor

Please note that there are a few failures in the tests...testQueryOverlappingMultipleIntervals fails in 3 cases (probably because they were written using the wrong definition of "abutting" :-)

fix: Fix tests for previous behaviour of abuts method
refactor: Change abuts method name to endsAtStartOf
@yfarjoun
Copy link
Contributor

This looks better to me. @tfenne ?

@tfenne
Copy link
Member

tfenne commented Sep 12, 2017

LGTM. I think the failed check is due to a random error contacting ENA in one of the tests. I've restarted it. Once it passes I'll merge.

@tfenne tfenne merged commit 04b17d5 into samtools:master Sep 12, 2017
@codecov-io
Copy link

Codecov Report

Merging #979 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@              Coverage Diff               @@
##              master      #979      +/-   ##
==============================================
- Coverage     65.938%   65.927%   -0.01%     
  Complexity      7459      7459              
==============================================
  Files            529       529              
  Lines          31956     31964       +8     
  Branches        5460      5462       +2     
==============================================
+ Hits           21071     21073       +2     
- Misses          8743      8749       +6     
  Partials        2142      2142
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/samtools/QueryInterval.java 55.556% <0%> (-12.012%) 14 <2> (ø)
src/main/java/htsjdk/samtools/BAMFileReader.java 63.714% <0%> (ø) 39 <0> (ø) ⬇️
...a/htsjdk/samtools/cram/encoding/rans/Decoding.java 82.143% <0%> (-3.571%) 11% <0%> (-1%)
...sjdk/samtools/util/Md5CalculatingOutputStream.java 76.923% <0%> (+7.692%) 9% <0%> (+1%) ⬆️

@Kirvolque Kirvolque changed the title Optimize intervals falls to merge abutting intervals Optimize intervals fails to merge abutting intervals May 2, 2018
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