Skip to content

Changed cr.igv.seg output of ModelSegments to give log2 Segment_Mean. #5976

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

Merged
merged 2 commits into from
May 31, 2019

Conversation

samuelklee
Copy link
Contributor

@samuelklee samuelklee commented May 30, 2019

Also changed LegacySegmentCollection and CalledLegacySegmentCollection writers to overwrite instead of append. (This was fixed for AbstractRecordCollection writers in an earlier PR but was missed in these collections that override the default writer.)

See https://gatkforums.broadinstitute.org/gatk/discussion/24048/a-few-things-need-help-about-output-of-command-modelsegments#latest for some context. In principle, IGV should be able to handle linear scaling, but it scales automatically for seg files and I'm not sure if you can get around it without some additional steps. Seems easier to just output log2 copy ratios. (Perhaps we initially output linear copy ratios to maintain backwards compatibility with previous versions of GATK CNV? If so, the *LegacySegmentCollections seem to be pulling double duty, since we use them to provide IGV compatibility. In any case, we are currently inconsistent between ModelSegments and CallCopyRatioSegments, as I noted in the forum post.)

@LeeTL1220 any objections? If downstream scripts are consuming the IGV output of ModelSegments, they should be adjusted if necessary.

@samuelklee samuelklee requested a review from LeeTL1220 May 30, 2019 14:03
@codecov
Copy link

codecov bot commented May 30, 2019

Codecov Report

Merging #5976 into master will increase coverage by 0.001%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##              master    #5976       +/-   ##
==============================================
+ Coverage     86.929%   86.93%   +0.001%     
  Complexity     32714    32714               
==============================================
  Files           2013     2013               
  Lines         151268   151268               
  Branches       16604    16604               
==============================================
+ Hits          131496   131497        +1     
  Misses         13718    13718               
+ Partials        6054     6053        -1
Impacted Files Coverage Δ Complexity Δ
...ute/hellbender/tools/copynumber/ModelSegments.java 97.059% <100%> (ø) 41 <0> (ø) ⬇️
...ats/collections/CalledLegacySegmentCollection.java 63.043% <100%> (ø) 4 <0> (ø) ⬇️
...r/formats/collections/LegacySegmentCollection.java 72.973% <100%> (ø) 4 <0> (ø) ⬇️
...nder/utils/runtime/StreamingProcessController.java 67.773% <0%> (+0.474%) 33% <0%> (ø) ⬇️

@LeeTL1220
Copy link
Contributor

@samuelklee I have no objection. I checked with one collaborator about the change to output format and he had no objection. He nor I knew of any downstream automation that uses the IGV, but this is a pretty informal check.

Please make sure that this change gets included in the release notes!

Copy link
Contributor

@LeeTL1220 LeeTL1220 left a comment

Choose a reason for hiding this comment

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

Fee free to merge if tests pass.

@samuelklee
Copy link
Contributor Author

Thanks, @LeeTL1220! @droazen please include in the next release notes.

@samuelklee samuelklee merged commit 051e00d into master May 31, 2019
@samuelklee samuelklee deleted the sl_log2_igv branch May 31, 2019 19:07
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