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

Changed exact-match tests for ModelSegments to allow for numerical differences due to Java version. #8111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samuelklee
Copy link
Contributor

@samuelklee samuelklee commented Nov 29, 2022

Closes #8107.

@droazen
Copy link
Collaborator

droazen commented Nov 29, 2022

Maybe it would be better to pair this with the upcoming Java 17 upgrade, so that we don't have to explicitly check the Java version? What do you think @cmnbroad ?

@gatk-bot
Copy link

Github actions tests reported job failures from actions build 3577583009
Failures in the following jobs:

Test Type JDK Job ID Logs
integration 11.0.11+9 3577583009.12 logs

@samuelklee samuelklee force-pushed the sl_modelsegments_java_version_dependent_expected branch from 4a36405 to bc9b5a7 Compare November 29, 2022 20:47
@cmnbroad
Copy link
Collaborator

I suppose we can wait, but I still think there is some inherent instability that is transient that we're going to have to deal with at some point. It looks like the last time I ran the CI tests on the Java 17 branch, these tests were failing on the docker job but passing on the the non-docker job, both with (probably different versions) of Java 17. I was hoping that narrowing it down to a small range of Java versions might help.

@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #8111 (bc9b5a7) into master (ba7678e) will increase coverage by 2.612%.
The diff coverage is 70.000%.

Additional details and impacted files
@@               Coverage Diff               @@
##              master     #8111       +/-   ##
===============================================
+ Coverage     84.031%   86.643%   +2.612%     
- Complexity     37445     38950     +1505     
===============================================
  Files           2335      2335               
  Lines         182679    182684        +5     
  Branches       20053     20053               
===============================================
+ Hits          153507    158283     +4776     
+ Misses         22124     17367     -4757     
+ Partials        7048      7034       -14     
Impacted Files Coverage Δ
...tools/copynumber/ModelSegmentsIntegrationTest.java 94.304% <70.000%> (-1.775%) ⬇️
.../hellbender/utils/python/PythonUnitTestRunner.java 75.410% <0.000%> (-3.279%) ⬇️
...itute/hellbender/tools/LocalAssemblerUnitTest.java 92.448% <0.000%> (ø)
...ellbender/tools/dragstr/CalibrateDragstrModel.java 70.345% <0.000%> (+0.230%) ⬆️
...rg/broadinstitute/hellbender/utils/io/IOUtils.java 74.728% <0.000%> (+0.272%) ⬆️
.../hellbender/tools/reference/CompareReferences.java 80.237% <0.000%> (+0.395%) ⬆️
...tools/walkers/rnaseq/GeneExpressionEvaluation.java 89.286% <0.000%> (+0.397%) ⬆️
...copynumber/utils/segmentation/KernelSegmenter.java 97.845% <0.000%> (+0.431%) ⬆️
.../tools/walkers/variantutils/FamilyLikelihoods.java 81.304% <0.000%> (+0.435%) ⬆️
.../broadinstitute/hellbender/utils/MannWhitneyU.java 94.144% <0.000%> (+0.450%) ⬆️
... and 170 more

@samuelklee
Copy link
Contributor Author

See some of my findings about numerical differences across 8/11/17 and possible causes in this old Slack thread: https://broadinstitute.slack.com/archives/C1HH1V5EC/p1657634295565369 We’re starting to get into some relatively hairy issues there, IMO!

But just in case it wasn’t clear: 1) None of these numerical differences should be scientifically concerning in the end, and 2) I think we still have numerical reproducibility within each fixed Java version (although if we happen to see any evidence to the contrary, please point to them here). So I don’t think we have too much to worry about once the test infrastructure settles.

@samuelklee
Copy link
Contributor Author

Also, looks like tests are passing, so the runtime version check seems to have done the trick. I’ll leave it up to you both what the next steps should be, no strong opinions on my end!

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 this pull request may close these issues.

ModelSegments integration test failures on newer Java 11 releases
4 participants