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.

Sign up for GitHub

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

Sam record pair identification depends on required SAM fields #994

Closed
wants to merge 4 commits into from
Closed

Sam record pair identification depends on required SAM fields #994

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba91430
feat: Pair identification of two sam records
DenisVerkhoturov Sep 21, 2017
272634a
feat: Pair identification tested
DenisVerkhoturov Sep 21, 2017
7843453
refactor: Pair identification test moved to SAMRecordUnitTest
DenisVerkhoturov Sep 21, 2017
3c01a88
fix: Add QNAME in pair identification
DenisVerkhoturov Sep 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/main/java/htsjdk/samtools/SAMRecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,19 @@ private boolean hasReferenceName() {
return hasReferenceName(mReferenceIndex, mReferenceName);
}

/**
* @return {@code true} if records belong to the same pairwise alignment
*/
public boolean isPair(final SAMRecord record) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about this function, a mate is another record with the same query name...why are you checking everything else, and not the QNAME? I'm sure there's a reason, but it should be explained in the javadoc as it is not obvious at all!

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the remark, I think it does matter in SamRecord and I fixed it, but if we implement it in HitsForInsert it doesn't matter because it deals records with the same QNAME only. Here is the logic that produces HitsForInsert.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to move this to SamPairUtil, because there are already a lot of methods in SAMRecord that may be better to be used within an utility class. Eventually, reads would be accessed by an interface (e.g., #985) and thus this methods may live in an utility class that is implementation-independent.

return record != null
&& this.getMateAlignmentStart() == record.getAlignmentStart()
&& record.getMateAlignmentStart() == this.getAlignmentStart()
&& !NO_ALIGNMENT_REFERENCE_NAME.equals(this.getMateReferenceName())
&& !NO_ALIGNMENT_REFERENCE_NAME.equals(record.getMateReferenceName())
&& Objects.equals(this.getReferenceName(), record.getMateReferenceName())
&& Objects.equals(record.getReferenceName(), this.getMateReferenceName());
}

/**
* @return true if this SAMRecord has a mate reference, either as a String or index (or both).
*/
Expand Down
54 changes: 54 additions & 0 deletions src/test/java/htsjdk/samtools/SAMRecordTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package htsjdk.samtools;

import org.testng.Assert;
import org.testng.annotations.Test;

public class SAMRecordTest {
Copy link
Contributor

@pshapiro4broad pshapiro4broad Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately it looks like SAMRecordUnitTest is the class where tests for SAMRecord are stored. If so, you should either move those tests here or these tests there.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, done here.


@Test
public void testRecordsArePairIfTheyLinkEachOtherInMateFields() {
final SAMRecord first = new SAMRecord(new SAMFileHeader(new SAMSequenceDictionary()));
first.setAlignmentStart(42);
first.setReferenceName("chrm1");
final SAMRecord second = new SAMRecord(new SAMFileHeader(new SAMSequenceDictionary()));
second.setAlignmentStart(142);
second.setReferenceName("chrm2");

first.setMateAlignmentStart(second.getAlignmentStart());
first.setMateReferenceName(second.getReferenceName());
second.setMateAlignmentStart(first.getAlignmentStart());
second.setMateReferenceName(first.getReferenceName());

Assert.assertTrue(first.isPair(second));
Assert.assertTrue(second.isPair(first));
}

@Test
public void testRecordsArePairIfTheyHaveNoMateFields() {
final SAMRecord first = new SAMRecord(new SAMFileHeader(new SAMSequenceDictionary()));
first.setAlignmentStart(42);
first.setReferenceName("chrm1");
final SAMRecord second = new SAMRecord(new SAMFileHeader(new SAMSequenceDictionary()));
second.setAlignmentStart(142);
second.setReferenceName("chrm2");

Assert.assertFalse(first.isPair(second));
Assert.assertFalse(second.isPair(first));
}

@Test
public void testRecordsArePairIdentificationDoesNotThrowNpe() {
final SAMRecord first = new SAMRecord(new SAMFileHeader(new SAMSequenceDictionary()));

Assert.assertFalse(first.isPair(null));
}

@Test
public void testRecordsArePairIdentificationDoesNotThrowNpeIfFieldsAreUndefined() {
final SAMRecord first = new SAMRecord(new SAMFileHeader(new SAMSequenceDictionary()));
final SAMRecord second = new SAMRecord(new SAMFileHeader(new SAMSequenceDictionary()));

Assert.assertFalse(first.isPair(second));
Assert.assertFalse(second.isPair(first));
}
}