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

CondenseDepthEvidence tool #7926

Merged
merged 1 commit into from
Jul 6, 2022
Merged

CondenseDepthEvidence tool #7926

merged 1 commit into from
Jul 6, 2022

Conversation

tedsharpe
Copy link
Contributor

No description provided.

@tedsharpe tedsharpe requested a review from mwalker174 June 29, 2022 20:42
@tedsharpe
Copy link
Contributor Author

Very small PR, as promised.

@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #7926 (4f58dae) into master (8341ae5) will increase coverage by 0.015%.
The diff coverage is 88.235%.

❗ Current head 4f58dae differs from pull request most recent head 8c24baa. Consider uploading reports for the commit 8c24baa to get more accurate results

@@               Coverage Diff               @@
##              master     #7926       +/-   ##
===============================================
+ Coverage     87.072%   87.086%   +0.015%     
+ Complexity     37002     36990       -12     
===============================================
  Files           2221      2217        -4     
  Lines         173927    173821      -106     
  Branches       18793     18791        -2     
===============================================
- Hits          151441    151374       -67     
+ Misses         15850     15817       -33     
+ Partials        6636      6630        -6     
Impacted Files Coverage Δ
...ute/hellbender/tools/sv/CondenseDepthEvidence.java 80.488% <80.488%> (ø)
...itute/hellbender/utils/io/FeatureOutputStream.java 85.366% <100.000%> (ø)
...tools/sv/CondenseDepthEvidenceIntegrationTest.java 100.000% <100.000%> (ø)
...broadinstitute/hellbender/utils/UtilsUnitTest.java 93.151% <0.000%> (-0.458%) ⬇️
...er/tools/walkers/variantutils/VariantsToTable.java 93.333% <0.000%> (-0.063%) ⬇️
...itute/hellbender/tools/LocalAssemblerUnitTest.java 92.448% <0.000%> (ø)
...s/variantutils/VariantsToTableIntegrationTest.java 100.000% <0.000%> (ø)
...s/solver/SynchronizedUnivariateSolverUnitTest.java
...r/utils/solver/UnivariateSolverSpecifications.java
...r/utils/solver/UnivariateSolverJobDescription.java
... and 6 more

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 @tedsharpe, the succinctness of this is nice to see and shows how much it pays off to have proper libraries for the sv evidence types. I compared output against the old CondenseReadCounts tool and results seem nearly identical. The only exception being that your tools includes partial bins.

This is generally better, but there may be some cases where this causes numerical issues with gCNV, such as having extremely small bins. To allow us to move in baby steps, can you also add a --min-interval-size parameter, default to 0, and just drop bins below this value. Maybe validate that the given min <= max as well.

Can you also incorporate that into the testing? It appears the test file doesn't have any "partial" bins, but I think that's important behavior to check for.

Comment on lines 133 to 137
final int[] accumCounts = accumulator.getCounts();
final int[] featureCounts = feature.getCounts();
for ( int idx = 0; idx != accumCounts.length; ++idx ) {
accumCounts[idx] += featureCounts[idx];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final int[] accumCounts = accumulator.getCounts();
final int[] featureCounts = feature.getCounts();
for ( int idx = 0; idx != accumCounts.length; ++idx ) {
accumCounts[idx] += featureCounts[idx];
}
MathUtils.addToArrayInPlace(accumulator.getCounts(), feature.getCounts());

@tedsharpe
Copy link
Contributor Author

Positive test demonstrates that the last row, which has nothing to be merged with, gets dropped when minimum interval length is 101 since its length is 100. (That's why the final row in merged.rd.txt is omitted in this commit.)
I believe all your comments have been addressed.

@tedsharpe
Copy link
Contributor Author

Oh, wait. Missed the addToArrayInPlace suggestion.

@tedsharpe
Copy link
Contributor Author

OK. Now.

@tedsharpe tedsharpe force-pushed the tws_CondenseDepthEvidence branch from 4f58dae to 8c24baa Compare July 6, 2022 16:55
@tedsharpe tedsharpe merged commit 88b0578 into master Jul 6, 2022
@tedsharpe tedsharpe deleted the tws_CondenseDepthEvidence branch July 6, 2022 18:22
orlicohen pushed a commit that referenced this pull request Jul 18, 2022
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.

2 participants