-
Notifications
You must be signed in to change notification settings - Fork 597
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
Added check for non-finite copy ratios in ModelSegments pipeline. #4292
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4292 +/- ##
==============================================
+ Coverage 79.082% 79.27% +0.188%
- Complexity 16655 17036 +381
==============================================
Files 1050 1050
Lines 59681 60796 +1115
Branches 9754 9946 +192
==============================================
+ Hits 47197 48193 +996
- Misses 8696 8787 +91
- Partials 3788 3816 +28
|
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.
Can you just add a bit more for a user to diagnose?
@samuelklee I'm fine without this one in next release. |
ea18fc1
to
999e8bf
Compare
@LeeTL1220 OK, tweaked the message a bit. I think I'm OK with this going in for the next point release. This is the sort of thing for which it will be nice to have the automatic validations, as a sanity check. |
@samuelklee Is it possible to expand the error message a bit so that users get something actionable. For example, "Does the interval list match what was covered in the bam file? Does this bam have coverage in all intervals?" |
I'd rather keep the message more generic, and think of the check as simply defining what a valid A more appropriate place for the sort of message you suggest is in the relevant denoising method. In the edge case you encountered, you used a BAM that was almost completely uncovered in all bins at the specified resolution, resulting in a sample median of zero. Since one of the steps in standardization is dividing by the sample median, this results in a divide by zero. I've added the corresponding check. |
@LeeTL1220 Is this good to go? |
Go for it.
…On Feb 21, 2018 16:51, "samuelklee" ***@***.***> wrote:
@LeeTL1220 <https://github.com/leetl1220> Is this good to go?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4292 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACDXk1jxHtfgMbCXExlN3x5i6jPZYECkks5tXI_AgaJpZM4RxGuu>
.
|
@LeeTL1220 This should fail earlier in situations like the one you ran into with the low coverage test data. I don't think there should be any unintended side effects. I'm fine if this doesn't make it in before the point release if you want to run some sanity checks on real data with it (since users should be able to figure out what is going on relatively quickly if they run into the issue), but I'll leave it up to you.