Improving exception messages in AssemblyRegion #6781
Conversation
| Math.min(sequence.getSequenceLength(), genomeLoc.getEnd() + padding)).getBases(); | ||
| } | ||
|
|
||
| private static String listContigsAsString(final SAMSequenceDictionary sequenceDictionary) { |
There was a problem hiding this comment.
Move to SequenceDictionaryUtils (or, better, use one of the existing methods there like SequenceDictionaryUtils.getContigNamesList())
There was a problem hiding this comment.
Of course that exists already.
There was a problem hiding this comment.
It actually exists in at least 2 places. ReadUtils.prettyPrintSequenceRecords seems to be the version I want.
| @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "Contig chrNotInReference not found in reference.*") | ||
| //Testing a case where an NPE was thrown when somehow the reference sequence dictionary didn't contain a contig in the | ||
| //assembly region. | ||
| public void testMimatchedReferenceAndRegion(){ |
There was a problem hiding this comment.
My keyboard is going to kill me with typos. It's started to just drop random letters and double others.
| "\nPlease check that you are using a compatible reference for your data." + | ||
| "\nReference Sequences: " + listContigsAsString(referenceReader.getSequenceDictionary())); | ||
|
|
||
| return referenceReader.getSubsequenceAt(contig, |
There was a problem hiding this comment.
getSubsequenceAt() will already throw a UserException.MissingContigInSequenceDictionary exception internally if the contig is missing -- is there some way we could rely on that existing check? Ideally we should avoid adding lots of additional sequence dictionary lookups to frequently-called methods if we can possibly help it...
There was a problem hiding this comment.
It's not clear to me how to avoid it because we need the sequence length from the sequence to make the call to getSubsequenceAt. This pr isn't adding a new lookup, it's just moving it. We could add a new sequenceDictionary method that automatically clips to the ends to avoid this extra lookup, but that seems like a lot of effort.
There was a problem hiding this comment.
Yes, you're right, it can't be avoided. This is fine then.
| @@ -122,6 +124,19 @@ private void assertGoodReferenceGetter(final byte[] actualBytes, final SimpleInt | |||
| return tests.subList(2,3).toArray(new Object[][]{}); //HACK! | |||
| } | |||
|
|
|||
| @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "Contig chrNotInReference not found in reference.*") | |||
There was a problem hiding this comment.
I did not know about expectedExceptionsMessageRegExp ! And now that I do, I have mixed feelings about it...throwing a more specific exception type is better than checking the exception message text itself.
There was a problem hiding this comment.
You're right... changing to UserException.MissingContigInSequenceDictionary.
| public void testMimatchedReferenceAndRegion(){ | ||
| final SAMSequenceDictionary sequenceDictionary = header.getSequenceDictionary(); | ||
| final String contigNotInReference = "chrNotInReference"; | ||
| sequenceDictionary.addSequence(new SAMSequenceRecord(contigNotInReference, 1000)); |
There was a problem hiding this comment.
Won't this change the sequence dictionary in the shared header instance used by all the tests in this class? Can you write this test in a way that doesn't modify shared data?
There was a problem hiding this comment.
Ugh. Yes it does. Changing. Good catch.
* Replacing some cases of NPE with IllegalArgumentException with a clear message.
lbergelson
force-pushed
the
lb_fix_npe_in_assemblyregion
branch
from
4b0c1ac to
20328e7
Compare
|
@droazen I think I addressed everything. Thanks for the review. Good catch on the header. |
…erence dictionary (#6781) * Improving exception messages in AssemblyRegion for contigs not in the reference dictionary * Replacing some cases of NPE with UserException.MissingContigInSequenceDictionary with a clear message.
This should help clarify the issue we talked about at the meeting today if it comes up again. I can't remember what the ticket was to link to though.