-
Notifications
You must be signed in to change notification settings - Fork 242
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
MergeSamFiles accept SO:UNKNOWN #1069
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -262,7 +262,7 @@ public SortOrder getSortOrder() { | |
try { | ||
return SortOrder.valueOf(so); | ||
} catch (IllegalArgumentException e) { | ||
log.warn("Found non conforming header SO tag: " + so + ". Treating as 'unknown'."); | ||
log.warn("Found non-conforming header SO tag: " + so + ". Treating as 'unknown'."); | ||
sortOrder = SortOrder.unknown; | ||
} | ||
} | ||
|
@@ -324,12 +324,18 @@ public void setAttribute(final String key, final Object value) { | |
*/ | ||
@Override | ||
public void setAttribute(final String key, final String value) { | ||
String tempVal = value; | ||
if (key.equals(SORT_ORDER_TAG)) { | ||
this.sortOrder = null; | ||
try { | ||
this.sortOrder = null; | ||
tempVal = SortOrder.valueOf(value).toString(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Also, it seems odd that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pshapiro4broad that's how the old implementation also does it...it let's the super-class deal with it...take a look at htsjdk.samtools.SAMFileHeader#getSortOrder, I think that might clarify things up (and note that this.sortOrder is private) |
||
} catch (IllegalArgumentException e) { | ||
tempVal = "unknown"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use |
||
} | ||
} else if (key.equals(GROUP_ORDER_TAG)) { | ||
this.groupOrder = null; | ||
} | ||
super.setAttribute(key, value); | ||
super.setAttribute(key, tempVal); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
import htsjdk.samtools.util.LineReader; | ||
import htsjdk.samtools.util.RuntimeIOException; | ||
import htsjdk.samtools.util.StringUtil; | ||
import htsjdk.samtools.util.Log; | ||
|
||
import java.io.BufferedWriter; | ||
import java.io.IOException; | ||
|
@@ -69,6 +70,8 @@ public class SAMTextHeaderCodec { | |
private static final Pattern FIELD_SEPARATOR_RE = Pattern.compile(FIELD_SEPARATOR); | ||
|
||
public static final String COMMENT_PREFIX = HEADER_LINE_START + HeaderRecordType.CO.name() + FIELD_SEPARATOR; | ||
private static final Log log = Log.getInstance(SAMTextHeaderCodec.class); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove extra newline |
||
|
||
void setWriter(final BufferedWriter writer) { | ||
this.writer = writer; | ||
|
@@ -233,8 +236,8 @@ private void parseHDLine(final ParsedHeaderLine parsedHeaderLine) { | |
try { | ||
if (soString != null) SAMFileHeader.SortOrder.valueOf(soString); | ||
} catch (IllegalArgumentException e) { | ||
reportErrorParsingLine(HEADER_LINE_START + parsedHeaderLine.getHeaderRecordType() + | ||
" line has non-conforming SO tag value: "+ soString + ".", | ||
reportErrorParsingLine(HEADER_LINE_START + parsedHeaderLine.getHeaderRecordType() + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. revert extra tab |
||
" line has non-conforming SO tag value: " + soString + ".", | ||
SAMValidationError.Type.HEADER_TAG_NON_CONFORMING_VALUE, null); | ||
} | ||
|
||
|
@@ -323,11 +326,30 @@ private class ParsedHeaderLine { | |
SAMValidationError.Type.HEADER_TAG_MULTIPLY_DEFINED, null); | ||
continue; | ||
} | ||
validateSortOrderValue(keyAndValue); | ||
mKeyValuePairs.put(keyAndValue[0], keyAndValue[1]); | ||
} | ||
lineValid = true; | ||
} | ||
|
||
private void validateSortOrderValue(String[] value) { | ||
if ("SO".equals(value[0])) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use SAMFileHeader.SORT_ORDER_TAG |
||
try { | ||
SAMFileHeader.SortOrder.valueOf(value[1]); | ||
} catch (IllegalArgumentException e) { | ||
if (validationStringency == ValidationStringency.STRICT) { | ||
throw new SAMFormatException("Found non conforming header SO tag: " | ||
+ value[1] | ||
+ ", exiting because VALIDATION_STRINGENCY=STRICT"); | ||
} else if (validationStringency == ValidationStringency.LENIENT) { | ||
log.warn("Found non conforming header SO tag: " | ||
+ value[1] + ". Treating as 'unknown'."); | ||
} | ||
value[1] = "unknown"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use SortOrder.unknown.toString() |
||
} | ||
} | ||
} | ||
|
||
/** | ||
* True if the line is recognized as one of the valid HeaderRecordTypes. | ||
*/ | ||
|
@@ -462,7 +484,7 @@ protected String getPGLine(final SAMProgramRecord programRecord) { | |
private void writeRGLine(final SAMReadGroupRecord readGroup) { | ||
println(getRGLine(readGroup)); | ||
} | ||
|
||
protected String getRGLine(final SAMReadGroupRecord readGroup) { | ||
final String[] fields = new String[2 + readGroup.getAttributes().size()]; | ||
fields[0] = HEADER_LINE_START + HeaderRecordType.RG; | ||
|
@@ -525,4 +547,4 @@ public void setValidationStringency(final ValidationStringency validationStringe | |
} | ||
this.validationStringency = validationStringency; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -552,12 +552,6 @@ public Object[][] tagCorrectlyProcessData() throws IOException { | |
"@RG\tID:0\tSM:Hi,Mom!\n" + | ||
"E\t147\tchr1\t15\t255\t10M\t=\t2\t-30\tCAACAGAAGC\t)'.*.+2,))\tU2:Z:CAA"; | ||
|
||
final String SOTagCorrectlyProcessTestData = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for your reply and sorry for the delay. We've removed this test case since as @a-filipp0v said, it's redundant after our implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure I understand...if it's just "Redundant" but not exactly equals, I'd ask that it be left in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This case does not pass now, since we no longer treat SO:NOTKNOWN as error as test expects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it |
||
"@HD\tVN:1.0\tSO:NOTKNOWN\n" + | ||
"@SQ\tSN:chr1\tLN:101\n" + | ||
"@RG\tID:0\tSM:Hi,Mom!\n" + | ||
"E\t147\tchr1\t15\t255\t10M\t=\t2\t-30\tCAACAGAAGC\t)'.*.+2,))\tU2:Z:CAA"; | ||
|
||
final String GOTagCorrectlyProcessTestData = | ||
"@HD\tVN:1.0\tGO:NOTKNOWN\n" + | ||
"@SQ\tSN:chr1\tLN:101\n" + | ||
|
@@ -568,7 +562,6 @@ public Object[][] tagCorrectlyProcessData() throws IOException { | |
{E2TagCorrectlyProcessTestData.getBytes(), SAMValidationError.Type.E2_BASE_EQUALS_PRIMARY_BASE}, | ||
{E2TagCorrectlyProcessTestData.getBytes(), SAMValidationError.Type.MISMATCH_READ_LENGTH_AND_E2_LENGTH}, | ||
{U2TagCorrectlyProcessTestData.getBytes(), SAMValidationError.Type.MISMATCH_READ_LENGTH_AND_U2_LENGTH}, | ||
{SOTagCorrectlyProcessTestData.getBytes(), SAMValidationError.Type.HEADER_TAG_NON_CONFORMING_VALUE}, | ||
{GOTagCorrectlyProcessTestData.getBytes(), SAMValidationError.Type.HEADER_TAG_NON_CONFORMING_VALUE} | ||
}; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move this line into the try clause?