Adapt CollectRrbsMetrics to SinglePassSamProgram #940

tcibinan · 2017-09-28T13:53:20Z

Description

CollectRrbsMetrics extends from CommandLineProgram. In order to optimize code and make it more readable SinglePassSamProgram was added.

Implementation

CRM extends from SPSP and can be used by CollectMultipleMetrics;
Add tests for running CRM by CollectMultipleMetrics;
Add more tests for CRM;
Argument OUTPUT in SPSP was made optional (it could be a point for a discussion) in case where CRM runs with argument METRICS_FILE_PREFIX instead of argument OUTPUT and 2 other related arguments CHART_OUTPUT and SUMMARY_OUTPUT:

java -jar picard.jar CollectRrbsMetrics
R=reference_sequence.fasta
I=input.bam
M=basename_for_metrics_output_files

Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

Added or modified tests to cover changes and any new functionality
Edited the README / documentation (if applicable)
All tests passing on Travis

Review

Final thumbs-up from reviewer
Rebase, squash and reword as applicable

coveralls · 2017-09-28T14:12:43Z

Coverage increased (+0.01%) to 76.783% when pulling 4e4c0c5 on EpamLifeSciencesTeam:epam-ls_CollectRrbsMetricsToSinglePassSamProgram into d4b04a9 on broadinstitute:master.

yfarjoun

Thanks for this @tcibinan. a few small comments and I think this is good!

yfarjoun · 2017-09-29T11:00:13Z

src/main/java/picard/analysis/CollectMultipleMetrics.java

+ @Override
+ public SinglePassSamProgram makeInstance(final String outbase, final String outext, final File input, final File reference, final Set<MetricAccumulationLevel> metricAccumulationLevel, final File dbSnp, final File intervals) {
+ final CollectRrbsMetrics program = new CollectRrbsMetrics();
+ program.OUTPUT = new File(outbase + ".rrbc.detail_metrics" + outext);


rrbc->rrbs (x3)

yfarjoun · 2017-09-29T11:01:50Z

src/main/java/picard/analysis/CollectRrbsMetrics.java

+ "<pre>" +
+ "java -jar picard.jar CollectRrbsMetrics \\<br />" +
+ " I=input.bam \\<br />" +
+ " O=detail_output.bam \\<br />" +


O=detail_output.txt, not .bam

yfarjoun · 2017-09-29T11:02:02Z

src/main/java/picard/analysis/CollectRrbsMetrics.java

+ " I=input.bam \\<br />" +
+ " O=detail_output.bam \\<br />" +
+ " CHART=chart_output.pdf \\<br />" +
+ " S=summary_output.bam \\<br />" +


.txt not .bam

yfarjoun · 2017-09-29T11:10:07Z

src/main/java/picard/analysis/SinglePassSamProgram.java

@@ -56,7 +56,7 @@
 @Argument(shortName = StandardOptionDefinitions.INPUT_SHORT_NAME, doc = "Input SAM or BAM file.")
 public File INPUT;

- @Argument(shortName = "O", doc = "File to write the output to.")
+ @Argument(shortName = "O", doc = "File to write the output to.", optional = true)


why is this optional?

Thanks for the review @yfarjoun.

CollectRrbsMetrics didn't have OUTPUT argument (only METRICS_FILE_PREFIX). But now it extends from SinglePassSamProgram and you can't run the metrics without mandatory argument OUTPUT.

So OUTPUT was made optional in order to be able to run CollectRrbsMetrics without it.

Also we can use that excessive OUTPUT argument with two more to specify output files explicitly. It can be useful and it is a way to use this unnecessary argument.

thinking about this more, this isn't "OK". this changes the behavior of all the other programs that extend from SinglePassSamProgram.

can you find a solution that doens't change the behavior/API for the other programs?

I owuld almost prefer to simply rename METRICS_FILE_PREFIX -> "OUTPUT" and to do away with the specific metric/chart/summary output variables....the only problem is that it would create an API change...

@cnorman, perhaps you can suggest solutions for this...given your knowledge of the parsing system.

I suggest that you implement it like CollectGcBiasMetrics is implemented, i.e. OUTPUT is for the detailed_metrics, SUMMARY_OUTPUT is for summary_metrics and CHART_OUTPUT for the pdf. You can use METRICS_FILE_PREFIX as Mutex, or remove it and change the API for this program (but not for all of the Single-Pass-Sam programs)

yfarjoun · 2017-09-29T11:14:33Z

why the change to specifying the outputs explicitly? what's the benefit there?

tcibinan · 2017-09-29T16:34:35Z

Please check the comment above.

yfarjoun · 2017-10-09T12:45:40Z

src/test/java/picard/metrics/CollectRrbsMetricsTest.java

@@ -1,102 +0,0 @@
-/*


why is this test removed? we should still be testing CollectRrbsMetrics directly, not only via collect multiple metrics.

yfarjoun · 2018-09-05T17:21:33Z

please respond to the review here if this is still interesting.

kbergin · 2019-04-16T17:33:35Z

Hi @tcibinan - Would you be available to complete this PR?

tcibinan · 2019-04-17T20:00:51Z

Hi! It's been a long time since I created the pull request. I'll try to find time to update the code to the latest changes and add requested changes but I can't be certain when. So if there is somebody who have enough courage to finish the request I'll be very pleased. Otherwise, I'll complete it one day.

yfarjoun · 2019-06-20T18:49:33Z

@tcibinan I'm wondering if this PR is needed now with the changes I made to CMM...perhaps you could re-tool CollectRrbsMetrics to only use the regular paramters from CMM and take additional parameter via EXTRA_ARGUMENTS?

kbergin · 2020-11-03T21:08:20Z

Hey @tcibinan ! Going through and looking at open PRs and issues in Picard and wanted to check in if you had seen Yossi's comment, or had any thoughts about the future of this PR?

Adapt CollectRrbsMetrics to SinglePassSamProgram

4e4c0c5

yfarjoun reviewed Sep 29, 2017

View reviewed changes

doc: Fix the few mistakes in docs

cfc1304

yfarjoun reviewed Oct 9, 2017

View reviewed changes

yfarjoun self-assigned this Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt CollectRrbsMetrics to SinglePassSamProgram #940

Adapt CollectRrbsMetrics to SinglePassSamProgram #940

tcibinan commented Sep 28, 2017 •

edited

Loading

coveralls commented Sep 28, 2017 •

edited

Loading

yfarjoun left a comment

yfarjoun Sep 29, 2017

tcibinan Oct 3, 2017

yfarjoun Sep 29, 2017

tcibinan Oct 3, 2017

yfarjoun Sep 29, 2017

tcibinan Oct 3, 2017

yfarjoun Sep 29, 2017

tcibinan Sep 29, 2017 •

edited

Loading

yfarjoun Oct 3, 2017

yfarjoun Oct 9, 2017

yfarjoun Oct 9, 2017

yfarjoun Dec 11, 2018

yfarjoun commented Sep 29, 2017

tcibinan commented Sep 29, 2017

yfarjoun Oct 9, 2017

yfarjoun commented Sep 5, 2018

kbergin commented Apr 16, 2019

tcibinan commented Apr 17, 2019

yfarjoun commented Jun 20, 2019

kbergin commented Nov 3, 2020

Adapt CollectRrbsMetrics to SinglePassSamProgram #940

Are you sure you want to change the base?

Adapt CollectRrbsMetrics to SinglePassSamProgram #940

Conversation

tcibinan commented Sep 28, 2017 • edited Loading

Description

Implementation

Checklist (never delete this)

Content

Review

coveralls commented Sep 28, 2017 • edited Loading

yfarjoun 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

tcibinan Sep 29, 2017 • 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

yfarjoun commented Sep 29, 2017

tcibinan commented Sep 29, 2017

Choose a reason for hiding this comment

yfarjoun commented Sep 5, 2018

kbergin commented Apr 16, 2019

tcibinan commented Apr 17, 2019

yfarjoun commented Jun 20, 2019

kbergin commented Nov 3, 2020

tcibinan commented Sep 28, 2017 •

edited

Loading

coveralls commented Sep 28, 2017 •

edited

Loading

tcibinan Sep 29, 2017 •

edited

Loading