(SV) clean up and add documentation to AssemblyContigAlignmentsConfigPicker #4971

SHuang-Broad · 2018-06-29T23:25:26Z

No description provided.

codecov-io · 2018-06-30T00:22:22Z

Codecov Report

Merging #4971 into master will decrease coverage by 0.157%.
The diff coverage is 97.222%.

@@               Coverage Diff               @@
##              master     #4971       +/-   ##
===============================================
- Coverage     86.352%   86.194%   -0.157%     
- Complexity     28592     29040      +448     
===============================================
  Files           1782      1782               
  Lines         132367    133721     +1354     
  Branches       14746     15130      +384     
===============================================
+ Hits          114301    115260      +959     
- Misses         12753     13065      +312     
- Partials        5313      5396       +83

Impacted Files	Coverage Δ	Complexity Δ
...iscovery/TestUtilsForAssemblyBasedSVDiscovery.java	`95.522% <100%> (+10.448%)`	`13 <0> (+1)`	⬆️
...covery/inference/TestUtilsCpxVariantInference.java	`99.512% <100%> (-0.021%)`	`7 <1> (ø)`
...lignment/AssemblyContigAlignmentsConfigPicker.java	`93.515% <91.667%> (+0.526%)`	`95 <27> (+8)`	⬆️
.../AssemblyContigAlignmentsConfigPickerUnitTest.java	`99.605% <99.429%> (+0.09%)`	`20 <14> (+2)`	⬆️
...der/tools/walkers/mutect/M2ArgumentCollection.java	`50% <0%> (-43.333%)`	`3% <0%> (ø)`
...itute/hellbender/tools/walkers/mutect/Mutect2.java	`52% <0%> (-40.857%)`	`18% <0%> (ø)`
...hellbender/tools/walkers/mutect/Mutect2Engine.java	`64.151% <0%> (-27.065%)`	`54% <0%> (+1%)`
...broadinstitute/hellbender/utils/IntervalUtils.java	`81.575% <0%> (-9.941%)`	`204% <0%> (+23%)`
...ava/org/broadinstitute/hellbender/utils/Utils.java	`73.376% <0%> (-8.156%)`	`238% <0%> (+80%)`
...tute/hellbender/utils/samples/SampleDBBuilder.java	`75% <0%> (-5%)`	`23% <0%> (+8%)`
... and 11 more

SHuang-Broad · 2018-06-30T16:41:40Z

Forgot to add information about effect of adding in the optional commit:

Based on the concordance tool, turning on/off the optional commit, the trade off is a handful of TP/FP.
That's why I made it optional.

cwhelan

Looks mostly fine; most of my comments are just wording changes to your documentation.

cwhelan · 2018-07-17T18:45:42Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

-     * or:
-     *  has more than 1 mapping, but only 1 mapping has MQ strictly above this threshold and it has a large gap in it.
+     * <p>
+     *     A filter that is used to remove contigs upfront which doesn't meet the following criteria


"which don't"

cwhelan · 2018-07-17T18:47:53Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

-     * Filters an input of SAM file containing alignments of a single-ended long read that
-     * aims at providing an "optimal coverage" of the assembly contig, based on an heuristic scoring scheme
+     * Filters input alignments of single-ended long reads, e.g. local assembly contigs,
+     * with the purpose being to provide "optimal coverage" of the assembly contig.


"with the objective of choosing a set of alignments that provide "optimal coverage" of the assembly contig"

cwhelan · 2018-07-17T18:48:36Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     *             first group the alignments of each local assembly contig together
+     *         </li>
+     *         <li>
+     *             then it exhaustively iterate through all possible combinations (named configuration) of the alignments,


"then exhaustively iterate"

cwhelan · 2018-07-17T18:49:53Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     *             there are multiple "optimal" configurations, and when this happens, all of them are returned
+     *         </li>
+     *         <li>
+     *             alignments containing large gaps are finally split into multiple ones


"are finally split at gap start and end locations"

this applies to insertions too, right? Maybe replace the word gap with "insertions and deletions"?

correct. by "gap" I mean insertion and deletions but not N's

cwhelan · 2018-07-17T18:50:34Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     *         <li>
+     *             alignments containing large gaps are finally split into multiple ones
+     *             (note, see warning in {@link #splitGaps(GoodAndBadMappings, boolean)})
+     *             A final round of pruning, in order to further trim away alignments


Should this be its own list item in the algorithm?

maybe "remove uninformative alignments" instead of "trim away alignments that offers more noise than signal"?

done and done

cwhelan · 2018-07-17T19:07:58Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     * picture: ref span by gapped original alignment   -------------          --------
+     *          ref span by overlapping other alignment                  ---------------------------
+     *
+     * TODO:


Can you expand on what the TODO is in the comment? Ie. "choose one of these options"..

Updated. What I wanted to say is that here we have two options, choose one, and evaluate (right now the evaluation says the choosing either is fine) when other changes are made.

cwhelan · 2018-07-17T19:08:33Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     *     better read coverage/breadth compared to the other, if so, keep the children alignments from the gap split,
+     *     and ditch the other alignment, otherwise ditch the alignment gapped original alignment,
+     *     the aim is to keep the split children alignments together
+     *  2) drop the gap-split children alignment whose read span is contained in other's.


"drop the gap-split child alignments whose read spans are contained in other alignment spans"

cwhelan · 2018-07-17T19:11:40Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+            if ( ! stateAndAlignment._1 )
+                continue;
+            final AlignmentInterval one = stateAndAlignment._2;
+            for (int j = 0; j < count; ++j) {


Could this be initialized at j = i + 1 to save work?

Yes! Thanks!

cwhelan · 2018-07-17T19:14:29Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

-            final int threshold) {
+    static GoodAndBadMappings splitGapsAndDropAlignmentContainedByOtherOnRead(final GoodAndBadMappings configuration) {
+        final List<AlignmentInterval> originalGoodMappings = configuration.goodMappings;
+        final List<Tuple2<Boolean, AlignmentInterval>> alignmentSplitChildren = originalGoodMappings.stream()


It seems weird to put these in a List<Tuple2<Boolean, AligmentInterval>>. Why not first just make a List<AlignmentInterval>. Then loop through it with i and j indices as below, but instead of resetting the tuple in this list just pop them into the good and bad lists below?

cwhelan · 2018-07-17T19:15:04Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

        }
    }

-    //==================================================================================================================
+    // functionality group 3.2: after gap split, further reclassify some good mapping as bad


I'd add a little description of which mappings are getting filtered out here to the comment.

added a short link to removeNonUniqueMappings(), which explains the criteria

SHuang-Broad

I've addressed all comments, Chris.
Can you take a look please? Thanks!

SHuang-Broad · 2018-07-19T00:41:46Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

-     * or:
-     *  has more than 1 mapping, but only 1 mapping has MQ strictly above this threshold and it has a large gap in it.
+     * <p>
+     *     A filter that is used to remove contigs upfront which doesn't meet the following criteria


SHuang-Broad · 2018-07-19T00:41:49Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

-     * Filters an input of SAM file containing alignments of a single-ended long read that
-     * aims at providing an "optimal coverage" of the assembly contig, based on an heuristic scoring scheme
+     * Filters input alignments of single-ended long reads, e.g. local assembly contigs,
+     * with the purpose being to provide "optimal coverage" of the assembly contig.


SHuang-Broad · 2018-07-19T00:41:52Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     *             first group the alignments of each local assembly contig together
+     *         </li>
+     *         <li>
+     *             then it exhaustively iterate through all possible combinations (named configuration) of the alignments,


SHuang-Broad · 2018-07-19T00:41:54Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     *             there are multiple "optimal" configurations, and when this happens, all of them are returned
+     *         </li>
+     *         <li>
+     *             alignments containing large gaps are finally split into multiple ones


correct. by "gap" I mean insertion and deletions but not N's

SHuang-Broad · 2018-07-19T00:41:56Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     *         <li>
+     *             alignments containing large gaps are finally split into multiple ones
+     *             (note, see warning in {@link #splitGaps(GoodAndBadMappings, boolean)})
+     *             A final round of pruning, in order to further trim away alignments


done and done

SHuang-Broad · 2018-07-19T00:42:42Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     * picture: ref span by gapped original alignment   -------------          --------
+     *          ref span by overlapping other alignment                  ---------------------------
+     *
+     * TODO:


Updated. What I wanted to say is that here we have two options, choose one, and evaluate (right now the evaluation says the choosing either is fine) when other changes are made.

SHuang-Broad · 2018-07-19T00:42:44Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+     *     better read coverage/breadth compared to the other, if so, keep the children alignments from the gap split,
+     *     and ditch the other alignment, otherwise ditch the alignment gapped original alignment,
+     *     the aim is to keep the split children alignments together
+     *  2) drop the gap-split children alignment whose read span is contained in other's.


SHuang-Broad · 2018-07-19T00:42:48Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

+            if ( ! stateAndAlignment._1 )
+                continue;
+            final AlignmentInterval one = stateAndAlignment._2;
+            for (int j = 0; j < count; ++j) {


Yes! Thanks!

SHuang-Broad · 2018-07-19T00:42:52Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

        }
    }

-    //==================================================================================================================
+    // functionality group 3.2: after gap split, further reclassify some good mapping as bad


added a short link to removeNonUniqueMappings(), which explains the criteria

SHuang-Broad · 2018-07-19T01:05:27Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

-            final int threshold) {
+    static GoodAndBadMappings splitGapsAndDropAlignmentContainedByOtherOnRead(final GoodAndBadMappings configuration) {
+        final List<AlignmentInterval> originalGoodMappings = configuration.goodMappings;
+        final List<Tuple2<Boolean, AlignmentInterval>> alignmentSplitChildren = originalGoodMappings.stream()


cwhelan · 2018-07-19T13:15:22Z

...tute/hellbender/tools/spark/sv/discovery/alignment/AssemblyContigAlignmentsConfigPicker.java

-                    return Utils.stream(split).map(ai -> new Tuple2<>(true, ai));
-                }).collect(Collectors.toList());
-        final int count = alignmentSplitChildren.size();
+        final List<AlignmentInterval> gapSplit = new ArrayList<>(originalGoodMappings.size());


Minor suggestion but I think formulating this as stream -> flatMap -> "collection as list" might be a little cleaner than creating the list upfront and then adding things to it inside forEaches.

cwhelan

👍

Looks good.

* clean up and bump test coverage * logic change: new option to keep gap-split alignments together or not, and opts to NOT to keep them together when one is contained by other alignments

SHuang-Broad added enhancement SV labels Jun 29, 2018

SHuang-Broad mentioned this pull request Jun 29, 2018

Switch to the updated type & location inference tool in SV pipeline #4111

Open

SHuang-Broad force-pushed the sh-sv-cleanup-aln-picker branch from 9c98466 to 9eb6dfe Compare July 10, 2018 16:17

SHuang-Broad mentioned this pull request Jul 10, 2018

Fix a new edge case in alignment deoverlapping #4962

Merged

SHuang-Broad force-pushed the sh-sv-cleanup-aln-picker branch 2 times, most recently from 8cf10a3 to 3028331 Compare July 12, 2018 15:00

cwhelan requested changes Jul 17, 2018

View reviewed changes

SHuang-Broad commented Jul 19, 2018

View reviewed changes

cwhelan reviewed Jul 19, 2018

View reviewed changes

cwhelan approved these changes Jul 19, 2018

View reviewed changes

(SV) refactor AssemblyContigAlignmentsConfigPicker

93ef08f

* clean up and bump test coverage * logic change: new option to keep gap-split alignments together or not, and opts to NOT to keep them together when one is contained by other alignments

SHuang-Broad force-pushed the sh-sv-cleanup-aln-picker branch from 14868f3 to 93ef08f Compare July 19, 2018 14:06

SHuang-Broad merged commit 9c0a867 into master Jul 19, 2018

SHuang-Broad deleted the sh-sv-cleanup-aln-picker branch July 19, 2018 18:08

(SV) clean up and add documentation to AssemblyContigAlignmentsConfigPicker #4971

(SV) clean up and add documentation to AssemblyContigAlignmentsConfigPicker #4971

Conversation

SHuang-Broad commented Jun 29, 2018

codecov-io commented Jun 30, 2018 • edited Loading

Codecov Report

SHuang-Broad commented Jun 30, 2018

cwhelan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SHuang-Broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhelan left a comment

Choose a reason for hiding this comment

codecov-io commented Jun 30, 2018 •

edited

Loading