-
Notifications
You must be signed in to change notification settings - Fork 594
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
Restore array output in gCNV WDLs for efficient postprocessing. #5490
Conversation
@asmirnov239 may want to look things over to make sure I haven't screwed anything up, as well! |
Codecov Report
@@ Coverage Diff @@
## master #5490 +/- ##
===============================================
- Coverage 87.081% 87.081% -<.001%
- Complexity 31478 31482 +4
===============================================
Files 1929 1930 +1
Lines 145041 145052 +11
Branches 16074 16074
===============================================
+ Hits 126303 126312 +9
Misses 12891 12891
- Partials 5847 5849 +2
|
Just a reminder to @vruano, @asmirnov239, and myself that we should probably get this (and any other gCNV WDL improvements) in before 4.1. |
I'll kick off a test run on FC soon (the queue is currently a bit backed up)... |
b69a644
to
08ffd75
Compare
@vruano @asmirnov239 I think this is good to go, please review. |
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.
Thanks for fixing it @samuelklee . Looks good to me!
Reverts the reversion in #5225, this time addressing the lexicographical ordering issue in #5217 at the WDL level by simply renaming gCNV output at the command line. If desired, we can eventually change gCNV itself to output filenames that are robust against lexicographic ordering, but this is low priority in my opinion.
@vruano this is what we discussed last week. Tests pass on Travis, and I'm pretty sure this fix should work OK, but I have not done an actual run with enough samples to see the fix in action. Can I assign you to review once I get a chance to do this?
EDIT: Also went ahead and rolled an older PR #5304 into this one so I can test both at the same time.
Closes #4724.