Added TransientAttributes to GATKRead #5664

jamesemery · 2019-02-12T18:00:08Z

Resolves #5653

Blocked by #5607

codecov-io · 2019-02-12T19:36:41Z

Codecov Report

Merging #5664 into master will increase coverage by 42.5%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##              master     #5664      +/-   ##
==============================================
+ Coverage     44.339%   86.838%   +42.5%     
- Complexity     19805     32325   +12520     
==============================================
  Files           1985      1991       +6     
  Lines         147675    149341    +1666     
  Branches       16235     16483     +248     
==============================================
+ Hits           65477    129685   +64208     
+ Misses         77055     13647   -63408     
- Partials        5143      6009     +866

Impacted Files	Coverage Δ	Complexity Δ
...broadinstitute/hellbender/utils/read/GATKRead.java	`31.25% <ø> (+18.75%)`	`7 <0> (+3)`	⬆️
.../SAMRecordToGATKReadAdapterSerializerUnitTest.java	`100% <100%> (+94.118%)`	`4 <1> (+3)`	⬆️
...kers/haplotypecaller/ReferenceConfidenceModel.java	`92.952% <100%> (+2.643%)`	`86 <9> (+3)`	⬆️
...transforms/markduplicates/MarkDuplicatesSpark.java	`94.521% <100%> (ø)`	`36 <0> (ø)`	⬇️
...forms/markduplicates/MarkDuplicatesSparkUtils.java	`89.573% <100%> (+0.948%)`	`68 <0> (+1)`	⬆️
...lbender/utils/read/SAMRecordToGATKReadAdapter.java	`92.806% <100%> (+17.897%)`	`146 <1> (+38)`	⬆️
...ools/walkers/contamination/ContaminationModel.java	`87.5% <0%> (-4.891%)`	`39% <0%> (ø)`
...er/tools/funcotator/FuncotatorIntegrationTest.java	`84.052% <0%> (-2.075%)`	`114% <0%> (-2%)`
...walkers/haplotypecaller/HaplotypeCallerEngine.java	`78.547% <0%> (-0.074%)`	`74% <0%> (ø)`
.../broadinstitute/hellbender/engine/LocusWalker.java	`79.245% <0%> (ø)`	`15% <0%> (ø)`	⬇️
... and 1089 more

droazen · 2019-03-27T17:07:28Z

@jamesemery Once your ReferenceConfidence PR goes in, can you rebase this to remove the outdated/duplicated commit?

jamesemery · 2019-04-08T16:49:00Z

rebased the branch for your review @cmnbroad

cmnbroad

Looks pretty good - a few mostly minor changes and tests requested.

cmnbroad · 2019-04-09T20:22:17Z

...roadinstitute/hellbender/tools/spark/transforms/markduplicates/MarkDuplicatesSparkUtils.java

@@ -406,10 +406,10 @@ private static int countOpticalDuplicates(OpticalDuplicateFinder finder, List<Pa
                    metrics.updateMetrics(read);
                    // NOTE: we use the SAMRecord transientAttribute field here specifically to prevent the already
                    // serialized read from being parsed again here for performance reasons.
-                    if (((SAMRecordToGATKReadAdapter) read).getTransientAttribute(OPTICAL_DUPLICATE_TOTAL_ATTRIBUTE_NAME)!=null) {
+                    if (read.getTransientAttribute(OPTICAL_DUPLICATE_TOTAL_ATTRIBUTE_NAME)!=null) {


There is a corresponding setTransientAttribute call in MarkDuplicatesSpark that has a redundant SAMRecordToGATKReadAdapter cast that can now be removed.

Thank you, I missed that.

cmnbroad · 2019-04-09T20:28:15Z

src/main/java/org/broadinstitute/hellbender/utils/read/GATKRead.java

@@ -667,6 +667,14 @@ default int numCigarElements(){
     */
    byte[] getAttributeAsByteArray( final String attributeName );

+    /**
+     * This is used to access the transient attribute store in the underlying SAMRecord.


Since this is an interface, it should be prescriptive for implementers, and it definitely shouldn't mention SAMRecord. The language should match that used in the set method below, something like:

Suggested change

* This is used to access the transient attribute store in the underlying SAMRecord.

* This is used to access a transient attribute store provided by the underlying implementation. Transient attributes should not be serialized or written out with a record.

cmnbroad · 2019-04-09T20:30:03Z

src/main/java/org/broadinstitute/hellbender/utils/read/GATKRead.java

+     *
+     * NOTE: This is an advanced use case for GATKRead and you should probably use setAttribute() instead
+     * @param key key under which the value will be stored
+     * @param value value to be keyed upon


Suggested change

* @param value value to be keyed upon

* @param value value to stored

Still suggest this change ^^

cmnbroad · 2019-04-09T20:31:09Z

src/main/java/org/broadinstitute/hellbender/utils/read/GATKRead.java

+     * @param value value to be keyed upon
+     */
+    public void setTransientAttribute(final Object key, final Object value);
+


There really should be a clearTransientAttribute method as well (SAMRecord has one so the GATK read adapter should pass that through).

This needs a basic set/get/clear test in GATKReadAdaptersUnitTest, and a roundtrip test in SAMRecordToGATKReadAdapterSerializerUnitTest to make sure we're really not serializing the attributes.

Done. (not I haven't added a clearTransientAttributes() call because it doesn't exist in the underlying SAMRecord code)

@jamesemery see SAMRecord.removeTransientAttribute

@cmnbroad To clarify, I did not add a clearTransientAttribute_s_() method. There is no exposed sam record method for removing ALL the transient attributes at once like there is for normal attributes.

jamesemery · 2019-04-12T15:38:39Z

@cmnbroad Responded to your review comments

cmnbroad

A couple of minor doc fixes, otherwise looks good. Feel free to merge when remaining comments are addressed.

@jamesemery - also note there was what appears to be a transient error on the last travis run, so we should make sure tests pass again before merging

cmnbroad · 2019-04-18T15:54:46Z

src/main/java/org/broadinstitute/hellbender/utils/read/SAMRecordToGATKReadAdapter.java

+    @Override
+    public void clearTransientAttribute( final String attributeName ) {
+        clearCachedValues();
+        samRecord.setTransientAttribute(attributeName, null);


Hm, I was actually thinking we would call samRecord.removeTransientAttribute, but maybe this is more consistent with the rest of the GATKRead interface.

Hmm... I was basing this off of our behavior for clearing regular attributes, i will change it to use the underlying samrecord method.

cmnbroad · 2019-04-18T16:11:13Z

src/main/java/org/broadinstitute/hellbender/utils/read/GATKRead.java

@@ -667,6 +667,14 @@ default int numCigarElements(){
     */
    byte[] getAttributeAsByteArray( final String attributeName );

+    /**
+     * This is used to access a transient attribute store provided by the underlying implementation. Transient attributes should not be serialized or written out with a record.


Transient attributes should not -> Transient attributes will not

cmnbroad · 2019-04-18T16:11:38Z

src/main/java/org/broadinstitute/hellbender/utils/read/GATKRead.java

+    /**
+     * This is used to access a transient attribute store provided by the underlying implementation. Transient attributes should not be serialized or written out with a record.
+     *
+     * NOTE: This is an advanced use case for GATKRead and you should probably use setAttribute() instead


Since this doc is on getTransientAttribute , setAttribute -> getAttribute

cmnbroad · 2019-04-18T16:12:12Z

src/main/java/org/broadinstitute/hellbender/utils/read/GATKRead.java

+     *
+     * NOTE: This is an advanced use case for GATKRead and you should probably use setAttribute() instead
+     * @param key key under which the value will be stored
+     * @param value value to be keyed upon


Still suggest this change ^^

jamesemery requested a review from lbergelson February 12, 2019 18:00

jamesemery assigned lbergelson Feb 12, 2019

droazen requested review from droazen and removed request for lbergelson March 15, 2019 17:15

droazen assigned droazen and unassigned lbergelson Mar 15, 2019

jamesemery added 2 commits April 8, 2019 12:42

Pushed the transient attribute method into the GATKRead interface

ea68574

fixed a typo

6bc12eb

jamesemery force-pushed the je_addTransientAttributeFieldToGATKRead branch from dcbb452 to 6bc12eb Compare April 8, 2019 16:48

droazen requested review from cmnbroad and removed request for droazen April 8, 2019 18:32

droazen assigned cmnbroad and unassigned droazen Apr 8, 2019

cmnbroad requested changes Apr 9, 2019

View reviewed changes

responded to review comments

07a6806

cmnbroad approved these changes Apr 18, 2019

View reviewed changes

droazen assigned jamesemery and unassigned cmnbroad Apr 22, 2019

responding to final round of comments

ebfff7f

jamesemery merged commit aa19925 into master Apr 22, 2019

jamesemery deleted the je_addTransientAttributeFieldToGATKRead branch April 22, 2019 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added TransientAttributes to GATKRead #5664

Added TransientAttributes to GATKRead #5664

jamesemery commented Feb 12, 2019

codecov-io commented Feb 12, 2019 •

edited

Loading

droazen commented Mar 27, 2019

jamesemery commented Apr 8, 2019

cmnbroad left a comment

cmnbroad Apr 9, 2019

jamesemery Apr 12, 2019

cmnbroad Apr 9, 2019

cmnbroad Apr 9, 2019

cmnbroad Apr 18, 2019

cmnbroad Apr 9, 2019

cmnbroad Apr 9, 2019

jamesemery Apr 12, 2019

cmnbroad Apr 12, 2019

jamesemery Apr 12, 2019

jamesemery commented Apr 12, 2019

cmnbroad left a comment •

edited

Loading

cmnbroad Apr 18, 2019

jamesemery Apr 22, 2019

cmnbroad Apr 18, 2019

cmnbroad Apr 18, 2019

cmnbroad Apr 18, 2019

	* This is used to access the transient attribute store in the underlying SAMRecord.
	* This is used to access a transient attribute store provided by the underlying implementation. Transient attributes should not be serialized or written out with a record.

	* @param value value to be keyed upon
	* @param value value to stored

Added TransientAttributes to GATKRead #5664

Added TransientAttributes to GATKRead #5664

Conversation

jamesemery commented Feb 12, 2019

codecov-io commented Feb 12, 2019 • edited Loading

Codecov Report

droazen commented Mar 27, 2019

jamesemery commented Apr 8, 2019

cmnbroad 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

jamesemery commented Apr 12, 2019

cmnbroad left a comment • edited Loading

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

codecov-io commented Feb 12, 2019 •

edited

Loading

cmnbroad left a comment •

edited

Loading