SV test and utils cleanup #5116
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5116 +/- ##
==========================================
Coverage ? 86.707%
Complexity ? 29097
==========================================
Files ? 1810
Lines ? 134816
Branches ? 14939
==========================================
Hits ? 116895
Misses ? 12521
Partials ? 5400
|
| if (expectedVCFPath == null) { | ||
| expectedVcs = Collections.emptyList(); | ||
| } else { | ||
| try (final VCFFileReader fileReader = new VCFFileReader(new File(expectedVCFPath), false)) { |
There was a problem hiding this comment.
Nitpick, but you can try-with-resources with multiple resources in one statement:
try (final VCFFileReader fileReader = new VCFFileReader(new File(expectedVCFPath), false);
final CloseableIterator iterator = fileReader.iterator()) {
...
There was a problem hiding this comment.
learned something. Thanks!
| null, | ||
| Collections.emptyList(), | ||
| false); | ||
|
|
There was a problem hiding this comment.
I personally found it just slightly complicated to follow the logic of how null expected paths would play out. Maybe worth just putting a comment here that null paths are interpreted as "no variants", so the test will be asserting that the produced variants file is empty.
There was a problem hiding this comment.
documented StructuralVariationDiscoveryPipelineSparkIntegrationTest.svDiscoveryVCFEquivalenceTest(...)
| @@ -150,6 +151,7 @@ public boolean requiresReads() { | |||
| protected void runTool(final JavaSparkContext ctx) { | |||
|
|
|||
| validateParams(); | |||
| final Set<VCFHeaderLine> defaultToolVCFHeaderLines = getDefaultToolVCFHeaderLines(); | |||
There was a problem hiding this comment.
Seems like there's no reason to assign to a local variable, just pass the result of the call directly to SvDiscoveryInputMetadata()
There was a problem hiding this comment.
local var removed
| } | ||
|
|
||
| public static SimpleInterval makeOneBpInterval(final String chr, final int pos) { | ||
| return new SimpleInterval(chr, pos, pos); |
There was a problem hiding this comment.
Maybe worth adding comment noting that SimpleIntervals are closed instead of semi-closed. I personally am occasionally confused by this difference.
There was a problem hiding this comment.
I think that's standard: SimpleInterval's, inherited from Locatable's from htsjdk, are 1-based, fully closed intervals. I learned this by doing some work on htsjdk.
There was a problem hiding this comment.
Added more doc for this method.
There was a problem hiding this comment.
Mostly looks good to me. I made a few nitpick comments.
The only remaining concern is that you created SVFileUtilsUnitTest (which is good) but two sets of functions there do not appear to have unit tests anywhere: readKmersFile/writeKmersFile, and readIntervalsFile/writeIntervalsFile. Given that the test paradigm would be pretty much identical to what you did for sam files, and the inverse pairs are right in the same class, I think it might be worth either banging out unit tests for them, or adding an issue to do so. The argument I see against it is that tools that use them have integration tests (except writeIntervalsFile which is totally unused). My feelings are mixed so I'm just going to bring it to your attention and ask you to think about it.
SHuang-Broad
force-pushed
the
sh-sv-test-and-utils-cleanup
branch
from
f5c1478
to
adf0bbb
Compare
|
@TedBrookings created ticket #5139 for the concerns you have |
* some clean up on util classes in SV packages and
get default headers into output VCF
* remove phantom sga-related test files and
move large test data files to the new sub-dir: src/test/resources/large/sv/
* integration test for new interpretation tool SvDiscoverFromLocalAssemblyContigAlignmentsSpark
SHuang-Broad
force-pushed
the
sh-sv-test-and-utils-cleanup
branch
from
adf0bbb
to
17ff58c
Compare
Does:
Note:
@TedBrookings tagging you as the victim for reviewing.
Please feel free to reassign.