samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.
http://samtools.github.io/htsjdk/
283 stars 242 forks source link

IntervalList not sorted? #1402

Open al-dunstan opened 5 years ago

al-dunstan commented 5 years ago

When constructing a IntervalListReferenceSequenceMask it looks like the intent is to sort the IntervalList that is passed as a parameter but, if the list isn't already sorted, it doesn't. It sorts, but ignores the result.

In samtools/util/IntervalListReferenceSequenceMask.java, about line 50:

public IntervalListReferenceSequenceMask(final IntervalList intervalList) {
        this.header = intervalList.getHeader();
        if (intervalList.getHeader().getSortOrder() != SAMFileHeader.SortOrder.coordinate) {
            intervalList.sorted(); // <---- here
        }

intervalList.sorted() returns a value (a sorted copy of the original), but here the returned value is ignored. There's a sort() method that sorts the list in-place, but it's deprecated (and not used).

yfarjoun commented 5 years ago

Nice catch!

Would you be interested in writing up the PR to fix this? Hopefully that PR would also include a small test that exposes the problem :-)

al-dunstan commented 5 years ago

I'll write one up, but I don't have a test case (sorry!). I found it by reading through the code. I'd jlooked at IntervalList just a moment before, saw the difference between sort() & sorted(), and realized 'that doesn't do anything'.

On Tue, Jul 30, 2019 at 4:49 PM Yossi Farjoun notifications@github.com wrote:

Nice catch!

Would you be interested in writing up the PR to fix this? Hopefully that PR would also include a small test that exposes the problem :-)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/samtools/htsjdk/issues/1402?email_source=notifications&email_token=AMYEFHBVPL5425RORRJXZC3QCCSN7A5CNFSM4IIACMW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3FIUBA#issuecomment-516590084, or mute the thread https://github.com/notifications/unsubscribe-auth/AMYEFHGUPJT2USHON273C6DQCCSN7ANCNFSM4IIACMWQ .

-- Al Dunstan Software Engineer Parabricks