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

Added gCNV integration test to detect numerical differences in the outputs. #7889

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

asmirnov239
Copy link
Collaborator

This PR expands the GermlineCNVCaller integration test suite and addresses #6893 and #4375. The tests that were added are:

  • Numerical accuracy test that checks for changes of gCNV model posterior values as compared to a previously computed model. This test is meant to detect Python library updates that affect gCNV results and unintentional consequences of minor gCNV model changes.
  • A test that runs gCNV in the COHORT mode with a pre-trained model as a starting point.
  • A test that runs gCNV with an annotated intervals file that contain GC content column.

As @samuelklee suggested we should consider adding functionality to the GermlineCNVCallerIntegrationTest to regenerate test files when there is a discrepancy in gCNV model outputs and we are okay with that discrepancy. See example of it in the HaplotypeCallerSparkIntegrationTest class -- specifically note UPDATE_EXACT_MATCH_EXPECTED_OUTPUTS flag. @mwalker174 let me know what you think.

runCommandLine(argsBuilder);
MODEL_FILES_TO_COMPARE.forEach(f -> CopyNumberTestUtils.assertFilesEqualUpToAllowedDeltaForDoubleValues(
new File(Paths.get(OUTPUT_DIR.getAbsolutePath(), outputPrefix + "-model").toString(), f),
new File(MODEL_EXACT_MATCH_EXPECTED_SUB_DIR, f),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I reused a previosly computed gCNV model for the numerical accuracy test. Good news is that the results haven't deviated since the time we made these test files. However, perhaps we should regenerate and create dedicated model files just for this test - especially to have a more stable test that compares denoised copy ratio values (see comment about the mu_denoised_copy_ratio_t.tsv).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes let's do what @samuelklee suggested and have a UPDATE_EXACT_MATCH_EXPECTED_OUTPUTS option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented that. Note that I had to move the test files into the new directory 'gcnv-numerical-accuracy'.


final List<String> MODEL_FILES_TO_COMPARE = Arrays.asList("log_q_tau_tk.tsv", "mu_ard_u_log__.tsv", "mu_psi_t_log__.tsv",
"std_ard_u_log__.tsv", "std_psi_t_log__.tsv", "mu_W_tu.tsv", "mu_log_mean_bias_t.tsv", "std_W_tu.tsv", "std_log_mean_bias_t.tsv");
final List<String> CALLS_FILES_TO_COMPARE = Arrays.asList("baseline_copy_number_t.tsv", "log_c_emission_tc.tsv",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the 'mu_denoised_copy_ratio_t.tsv' and 'std_denoised_copy_ratio_t.tsv' files from the numerical accuracy test because it was not passing with the ALLOWED_DELTA_FOR_DOUBLE_VALUES = 1E-6, however the values are not too different. I can potentially get around it by 1) regenerating the model, 2) taking more samples from the posterior or 3) setting the ALLOWED_DELTA_FOR_DOUBLE_VALUES to something smaller or some combination of the above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer (2)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the test pass now with the default value for the number of copy ratio samples.

@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #7889 (211aefb) into master (9ae1fd8) will decrease coverage by 1.679%.
The diff coverage is 17.647%.

❗ Current head 211aefb differs from pull request most recent head e4100fe. Consider uploading reports for the commit e4100fe to get more accurate results

@@               Coverage Diff               @@
##              master     #7889       +/-   ##
===============================================
- Coverage     86.954%   85.275%   -1.679%     
- Complexity     36897     37924     +1027     
===============================================
  Files           2214      2310       +96     
  Lines         173540    180395     +6855     
  Branches       18736     19841     +1105     
===============================================
+ Hits          150900    153831     +2931     
- Misses         16037     19755     +3718     
- Partials        6603      6809      +206     
Impacted Files Coverage Δ
...s/copynumber/GermlineCNVCallerIntegrationTest.java 16.346% <17.647%> (-72.333%) ⬇️
...adinstitute/hellbender/utils/R/RScriptLibrary.java 0.000% <0.000%> (-100.000%) ⬇️
...e/hellbender/utils/R/RScriptExecutorException.java 0.000% <0.000%> (-100.000%) ⬇️
...nder/metrics/analysis/AlleleFrequencyQCMetric.java 0.000% <0.000%> (-100.000%) ⬇️
...trics/analysis/BaseDistributionByCycleMetrics.java 0.000% <0.000%> (-100.000%) ⬇️
...OverlappingIntegerCopyNumberSegmentCollection.java 0.000% <0.000%> (-100.000%) ⬇️
.../CollectInsertSizeMetricsSparkIntegrationTest.java 4.878% <0.000%> (-95.122%) ⬇️
...der/utils/python/PythonScriptExecutorUnitTest.java 4.225% <0.000%> (-94.366%) ⬇️
...der/tools/walkers/vqsr/CNNVariantPipelineTest.java 5.882% <0.000%> (-94.118%) ⬇️
.../QualityScoreDistributionSparkIntegrationTest.java 5.970% <0.000%> (-94.030%) ⬇️
... and 348 more

runCommandLine(argsBuilder);
MODEL_FILES_TO_COMPARE.forEach(f -> CopyNumberTestUtils.assertFilesEqualUpToAllowedDeltaForDoubleValues(
new File(Paths.get(OUTPUT_DIR.getAbsolutePath(), outputPrefix + "-model").toString(), f),
new File(MODEL_EXACT_MATCH_EXPECTED_SUB_DIR, f),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes let's do what @samuelklee suggested and have a UPDATE_EXACT_MATCH_EXPECTED_OUTPUTS option.

@gatk-bot
Copy link

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

Test Type JDK Job ID Logs
conda 8 2762081645.3 logs

@asmirnov239 asmirnov239 force-pushed the as_add_gcnv_integration_tests branch 2 times, most recently from e4100fe to fb4611c Compare August 1, 2022 21:53
@asmirnov239 asmirnov239 marked this pull request as ready for review August 2, 2022 04:04
Copy link
Contributor

@mwalker174 mwalker174 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @asmirnov239 and @samuelklee for the numerical accuracy testing.

Do you need to include the elbo history files? They're a bit long. Otherwise, good to merge!

…tput of gCNV in the COHORT mode. Also added two minor tests covering different use cases.
…rationTest and regenerated test files for GermlineCNVCaller numerical accuracy integration tests.
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.

3 participants