Skip to content
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

endless while loop in estimateLibrarySize() in DuplicationMetrics.java #1146

Closed
zhangs3 opened this issue Mar 28, 2018 · 3 comments · Fixed by #1147
Closed

endless while loop in estimateLibrarySize() in DuplicationMetrics.java #1146

zhangs3 opened this issue Mar 28, 2018 · 3 comments · Fixed by #1147

Comments

@zhangs3
Copy link

zhangs3 commented Mar 28, 2018

Bug Report

Affected tool(s)

picard MarkDuplicates

Affected version(s)

Latest public release version: 2.18.0

Description

The while loop in the following codes (in the function estimateLibrarySize() in DuplicationMetrics.java) never ends for some initial values of M, c and n with which the function f() will never return a negative value.

while( f(M*c, c, n) >= 0 ) M *= 10.0;
 .
 .
 .
private static double f(double x, double c, double n) {
        return c/x - 1 + Math.exp(-n/x);
}

Steps to reproduce

Here is a short Java program to show the problem. The initial values of M, c and n are from real data.

public class Loop
{
    public static void main(String[] args)
    {
        // bug!!! f() never returns negative for initial values of
        double M = 100.0;
        double c = 357087881;
        double n = 357087883;

        while( f(M*c, c, n) >= 0 ) M *= 10.0; 
    }

    private static double f(double x, double c, double n) {
        System.err.println(c/x - 1 + Math.exp(-n/x));
        return c/x - 1 + Math.exp(-n/x);
    }
}

Expected behavior

The while loop should exit for certain value of M.

Actual behavior

For certain values of M, c and n, such as, 100.0, 357087881 and 357087883 respectively, the function f() never returns a negative value.

Suggested correction

while( f(M*c, c, n) > 0 ) M *= 10.0; 
M *= 10.0;  // if necessary
yfarjoun added a commit that referenced this issue Mar 29, 2018
- added a failing test (with the example from github #1146)
yfarjoun added a commit that referenced this issue Mar 29, 2018
- added a failing test (with the example from github #1146)
@yfarjoun
Copy link
Contributor

Thanks for this interesting edge case.

I took your example and made a test out of it, and also fixed it in line with your suggestion.
(PR here #1147)
Thanks again!

@zhangs3
Copy link
Author

zhangs3 commented Mar 29, 2018 via email

@yfarjoun
Copy link
Contributor

sorry to be a stickler,,,but this seems to be a separate issue...

yfarjoun pushed a commit that referenced this issue Apr 12, 2018
- white-space changes and comments
- added a failing test (with the example from github #1146)
- removed '=' sign from while loop. Once equality to zero is achieved, it's good enough for the bisection method.
- responding to review comments
ghost pushed a commit to DanieleBarreca/picard that referenced this issue Jun 8, 2018
- white-space changes and comments
- added a failing test (with the example from github broadinstitute#1146)
- removed '=' sign from while loop. Once equality to zero is achieved, it's good enough for the bisection method.
- responding to review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants