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

Test accuracy should be relaxed for picard.analysis.directed.CollectHsMetricsTest.testHsMetricsF80DoesNotUseCovCap #1987

Open
pgrt opened this issue Dec 26, 2024 · 3 comments

Comments

@pgrt
Copy link

pgrt commented Dec 26, 2024

Hello,

I am in the progress of upgrading the version of picard in Debian.

When packaging version 3.3.0 I met the following issue:

java.lang.AssertionError: expected [100000] but found [100000.0]
at org.testng.Assert.fail(Assert.java:94)
at org.testng.Assert.failNotEquals(Assert.java:513)
at org.testng.Assert.assertEqualsImpl(Assert.java:135)
at org.testng.Assert.assertEquals(Assert.java:116)
at org.testng.Assert.assertEquals(Assert.java:179)
at picard.analysis.directed.CollectHsMetricsTest.testHsMetricsF80DoesNotUseCovCap(CollectHsMetricsTest.java:202)

For sure, the change

--- a/src/test/java/picard/analysis/directed/CollectHsMetricsTest.java
+++ b/src/test/java/picard/analysis/directed/CollectHsMetricsTest.java
@@ -199,9 +199,9 @@
 
         IOUtil.deleteDirectoryTree(dir);
         // actual coverage should not be impacted by coverage_cap
-        Assert.assertEquals(highCoverageMetrics.MEAN_TARGET_COVERAGE, 100000);
-        Assert.assertEquals(highCoverageMetrics.MEDIAN_TARGET_COVERAGE, 100000);
-        Assert.assertEquals(highCoverageMetrics.FOLD_80_BASE_PENALTY, 1);
+        Assert.assertEquals(highCoverageMetrics.MEAN_TARGET_COVERAGE, 100000, 1.E-4);
+        Assert.assertEquals(highCoverageMetrics.MEDIAN_TARGET_COVERAGE, 100000, 1.E-4);
+        Assert.assertEquals(highCoverageMetrics.FOLD_80_BASE_PENALTY, 1, 1.E-4);
     }

fixes this, it is not an interesting problem but for sure we need to avoid it.

Thanks for maintaining picard,

Best wishes,
Pierre

@lbergelson
Copy link
Member

@pgrt Thanks for reporting this. I'm curious if changing the number it's comparing against from 100000 to double 100000.0 fixes it for you or only adding the epsilon?

lbergelson added a commit that referenced this issue Feb 11, 2025
* fix for #1987
* This should fix an issue when running tests on debian systems
@lbergelson
Copy link
Member

Thanks @pgrt I have a pr and will get this in our next release.

@pgrt
Copy link
Author

pgrt commented Feb 12, 2025

@pgrt Thanks for reporting this. I'm curious if changing the number it's comparing against from 100000 to double 100000.0 fixes it for you or only adding the epsilon?

Thanks for having considered my contribution (and others...)!

Yes, I confirm changing to 100000.0 works for me! I have just checked.

All the best,

Pierre

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

No branches or pull requests

2 participants