Yf provide arbtrary inputs to cmm #1337

yfarjoun · 2019-05-24T21:53:14Z

Adding the ability to specify individual arguments to programs in CollectMultipleMetrics.

This has been requested several times. for example #1333

@cmnbroad you might get a kick from this PR...:-)

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

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

…ics! - tested on InsertSizeMetrics

…parser

coveralls · 2019-05-28T16:33:01Z

Coverage increased (+0.5%) to 82.186% when pulling 7786318 on yf_provide_arbtrary_inputs_to_CMM into 0b95405 on master.

cmnbroad

A few minor comments, and one alternative approach to consider. It might be cleaner to just have the various makeInstance implementations construct a complete command line string or argument array out of the appropriate CMM args, plus the EXTRA_ARGS that are specific to that program, and call into a common method that allows the CLP to populate the entire program object the normal way. That would eliminate the need for the distributed mix of direct and CLP-based arg population; eliminate any validation weirdness or clobbering that might arise due to the fact that the CLP only sees a subset of the actual args, and simplify the implementation of doWork.

cmnbroad · 2019-05-28T20:13:33Z

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

    public boolean INCLUDE_UNPAIRED = false;

+    @Argument(doc="extra arguments to the various tools can be specified using the following format:" +
+            "<PROGRAM>::<ARGUMENT_AND_VALUE> where <PROGRAM> is one of the programs specified in PROGRAM, " +
+            "and <ARGUMENT_AND_VALUE> are the argument and value that you'd like to specify as you would on the commandline." +


commandline -> command line

cmnbroad · 2019-05-28T20:15:03Z

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

+            "<PROGRAM>::<ARGUMENT_AND_VALUE> where <PROGRAM> is one of the programs specified in PROGRAM, " +
+            "and <ARGUMENT_AND_VALUE> are the argument and value that you'd like to specify as you would on the commandline." +
+            "For example, to change the HISTOGRAM_WIDTH in CollectInsertSizeMetrics to 200 using the legacy parser, use:\n " +
+            "EXTRA_ARGUMENT=CollectInsertSizeMetrics::HISTOGRAM_WIDTH=200\n " +


AFAIK, the only public place where this doc gets published is with the GATK doc, where it will be incorrect, and the reference to legacy parser is irrelevant.

cmnbroad · 2019-05-28T20:28:26Z

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

+            }
+            final String argumentAndValue = matcher.group("argumentAndValue");
+
+


cmnbroad · 2019-05-28T20:33:23Z

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

+        for (String str : arguments) {
+            final Matcher matcher = pattern.matcher(str);
+            if (!matcher.matches()) {
+                throw new IllegalArgumentException("couldn't understand EXTRA_ARGUMENT " + str + " it doesn't conform to the regular expression given by " + pattern.pattern());


CommandLineException ?

I suppose it doesn't hurt, but I'm not sure displaying the regular expression is useful to the user.

cmnbroad · 2019-05-28T20:33:30Z

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

+            try {
+                program = Program.valueOf(programName);
+            } catch (IllegalArgumentException e) {
+                throw new IllegalArgumentException("Couldn't find program with value " + programName, e);


CommandLineException

cmnbroad · 2019-05-28T20:34:35Z

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

            instance.setDefaultHeaders(getDefaultHeaders());
            programs.add(instance);
        }
+        if(!additionalArguments.isEmpty()){
+            throw new IllegalArgumentException("EXTRA_ARGUMENT values were provided, but appropriate program wasn't requested:" +


It might make more sense to throw CommandLineException ?

appropriate -> corresponding ?

cmnbroad · 2019-05-29T13:31:22Z

src/test/java/picard/analysis/CollectMultipleMetricsTest.java

+    Object[][] extraArgumentValue() {
+                List<Object[]> tests = new ArrayList<>();
+
+                // this is actually legal after convertion to non-legacy


convertion -> conversion

cmnbroad · 2019-05-29T13:52:46Z

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

+            "and <ARGUMENT_AND_VALUE> are the argument and value that you'd like to specify as you would on the commandline." +
+            "For example, to change the HISTOGRAM_WIDTH in CollectInsertSizeMetrics to 200 using the legacy parser, use:\n " +
+            "EXTRA_ARGUMENT=CollectInsertSizeMetrics::HISTOGRAM_WIDTH=200\n " +
+            "Note that the following arguments cannot be modified on a per-program level: INPUT, REFERENCE_SEQUENCE, ASSUME_SORTED, and STOP_AFTER", optional = true)


What does "cannot be modified" mean in this case ? Will it fail, or is it just not advised ? Either way there should be code to specifically detect and reject these in the code ?

What happens if the user changes the METRIC_ACCUMULATION_LEVEL at the program level using this ?

cmnbroad · 2019-05-29T14:07:06Z

src/main/java/picard/cmdline/CommandLineProgram.java

-    private static String PROPERTY_USE_LEGACY_PARSER = "picard.useLegacyParser";
-    private static String PROPERTY_CONVERT_LEGACY_COMMAND_LINE = "picard.convertCommandLine";
+    public static String PROPERTY_USE_LEGACY_PARSER = "picard.useLegacyParser";
+    public static String PROPERTY_CONVERT_LEGACY_COMMAND_LINE = "picard.convertCommandLine";


Why do these need to be public ?

leftover from a different attempt...

cmnbroad · 2019-05-29T15:16:01Z

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

+            final String optionalValue = matcher.group("optionalValue");
+            if (optionalValue!=null && !optionalValue.equals("")) {
+                map.get(program).add(optionalValue);
+            }


This code path needs a test case, preferably one where another arg appears in the EXTRA_ARGS list after the optional one.

there is a test case...when there's a space after the "=" in

"EXTRA_ARGUMENT=CollectInsertSizeMetrics::HISTOGRAM_WIDTH= 58",

@yfarjoun I guess I misunderstood what the purpose of this code path is then. I interpreted "optional" to mean that this value was truly optional, but this is just to tolerate the space ? Although the test case passes as written (in that 58 gets propagated), this syntax won't work from the command line this way (without quoting at least - it does work with quoting).

But more importantly, although the tests seem to pass with both parsers (because args is already parsed in to an array of strings ?), I've tried a few variations manually and I haven't yet succeeded in passing EXTRA_ARGS using the Barclay parser (-Dpicard.useLegacyParser=false) from the command line at all. What is the right/intended syntax to use for extra args in that case ?

you'd need to quote the argument and the value together:

java -Dpicard.useLegacyParser=false -jar build/libs/picard.jar CollectMultipleMetrics \ -INPUT /Users/farjoun/Picard-public/testdata/picard/sam/summary_alignment_stats_test.sam \ -OUTPUT /var/folders/tc/hy9lszxd1dg9cf4bky51mrrd9k3s6g/T/alignmentMetrics6690349237641187401 \ -REFERENCE_SEQUENCE /Users/farjoun/Picard-public/testdata/picard/sam/summary_alignment_stats_test.fasta \ -METRIC_ACCUMULATION_LEVEL ALL_READS \ -PROGRAM null \ -PROGRAM CollectAlignmentSummaryMetrics \ -PROGRAM CollectInsertSizeMetrics \ -EXTRA_ARGUMENT "CollectInsertSizeMetrics::--HISTOGRAM_WIDTH 58" \ -EXTRA_ARGUMENT "CollectInsertSizeMetrics::--METRIC_ACCUMULATION_LEVEL LIBRARY" \ -EXTRA_ARGUMENT "CollectInsertSizeMetrics::--METRIC_ACCUMULATION_LEVEL READ_GROUP"

(I'll update the docs)

yfarjoun · 2019-05-30T01:36:24Z

Thanks for the review @cmnbroad. I think I responded to all your points.

yfarjoun · 2019-06-18T15:39:20Z

@gbggrant

gbggrant

👍

gbggrant · 2019-06-18T16:01:50Z

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

@@ -59,8 +73,8 @@
     * Includes a method for determining whether or not a Program explicitly needs a reference sequence (i.e. cannot be null)
     */

-    static final String USAGE_SUMMARY ="Collect multiple classes of metrics.  ";
-    static final String USAGE_DETAILS ="This 'meta-metrics' tool runs one or more of the metrics collection modules at the same" +
+    static final String USAGE_SUMMARY = "Collect multiple classes of metrics.  ";


Trailing white space.

gbggrant · 2019-06-18T16:04:03Z

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

    public boolean INCLUDE_UNPAIRED = false;

+    @Argument(doc="extra arguments to the various tools can be specified using the following format:" +
+            "<PROGRAM>::<ARGUMENT_AND_VALUE> where <PROGRAM> is one of the programs specified in PROGRAM, " +
+            "and <ARGUMENT_AND_VALUE> are the argument and value that you'd like to specify as you would on the command line." +


Add space (after the period) to end of this line for readability.

gbggrant · 2019-06-18T16:09:32Z

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

            instance.setDefaultHeaders(getDefaultHeaders());
            programs.add(instance);
        }
+        if(!additionalArguments.isEmpty()){


space between if and '(' and ')' and ' {'

gbggrant · 2019-06-18T16:11:26Z

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

+            "or, in the new parser:" +
+            "--EXTRA_ARGUMENT \"CollectInsertSizeMetrics::--HISTOGRAM_WIDTH 200\"\n " +
+            "(Quotes are required to avoid the shell from separating this into two arguments.) " +
+            "Note that the following arguments cannot be modified on a per-program level: INPUT, REFERENCE_SEQUENCE, ASSUME_SORTED, and STOP_AFTER." +


trailing space here too.

gbggrant · 2019-06-18T16:13:09Z

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

+            map.computeIfAbsent(program, (k) -> new ArrayList<>()).add(argumentAndValue);
+
+            final String optionalValue = matcher.group("optionalValue");
+            if (optionalValue != null && !optionalValue.equals("")) {


Use !StringUtils.isEmpty

- added the ability to specify extra arguments in CollectMultipleMetrics! - tested on InsertSizeMetrics - added some shenanigans for nonLegacy conversion... - some more work to make EXTRA_ARGUMENTS work on both legacy and new parser

Yossi Farjoun added 2 commits May 24, 2019 16:56

- added the ability to specify extra arguments in CollectMultipleMetr…

157e49c

…ics! - tested on InsertSizeMetrics

- added the ability to specify extra arguments in CollectMultipleMetr…

4bbaa94

…ics! - tested on InsertSizeMetrics

yfarjoun requested a review from cmnbroad May 24, 2019 21:53

Yossi Farjoun added 3 commits May 25, 2019 17:01

- added some shenanigans for nonLegacy convertion...

38a089c

- some more work to make EXTRA_ARGUMENTS work on both legacy and new …

e46f1a1

…parser

- fix tests

9cfaa44

cmnbroad reviewed May 29, 2019

View reviewed changes

- responding to review comments

4a5e85b

- respond to review comments

c3f0e21

gbggrant approved these changes Jun 18, 2019

View reviewed changes

- review comments

7786318

yfarjoun merged commit ce50809 into master Jun 18, 2019

yfarjoun deleted the yf_provide_arbtrary_inputs_to_CMM branch June 18, 2019 19:28

kbergin mentioned this pull request Nov 3, 2020

Feature rquest: CollectMultipleMetrics #1333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yf provide arbtrary inputs to cmm #1337

Yf provide arbtrary inputs to cmm #1337

yfarjoun commented May 24, 2019 •

edited

Loading

coveralls commented May 28, 2019 •

edited

Loading

cmnbroad left a comment

cmnbroad May 28, 2019

cmnbroad May 28, 2019

cmnbroad May 28, 2019

cmnbroad May 28, 2019

cmnbroad May 29, 2019

cmnbroad May 28, 2019

cmnbroad May 28, 2019

cmnbroad May 29, 2019

cmnbroad May 29, 2019

cmnbroad May 29, 2019

cmnbroad May 29, 2019

cmnbroad May 29, 2019

yfarjoun May 30, 2019

cmnbroad May 29, 2019

yfarjoun May 30, 2019

cmnbroad Jun 7, 2019 •

edited

Loading

yfarjoun Jun 17, 2019

yfarjoun Jun 17, 2019

yfarjoun commented May 30, 2019

yfarjoun commented Jun 18, 2019

gbggrant left a comment

gbggrant Jun 18, 2019

gbggrant Jun 18, 2019

gbggrant Jun 18, 2019

gbggrant Jun 18, 2019

gbggrant Jun 18, 2019

yfarjoun Jun 18, 2019

		}
		final String argumentAndValue = matcher.group("argumentAndValue");

Yf provide arbtrary inputs to cmm #1337

Yf provide arbtrary inputs to cmm #1337

Conversation

yfarjoun commented May 24, 2019 • edited Loading

Checklist (never delete this)

Content

Review

coveralls commented May 28, 2019 • edited Loading

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

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

cmnbroad Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun commented May 30, 2019

yfarjoun commented Jun 18, 2019

gbggrant 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

yfarjoun commented May 24, 2019 •

edited

Loading

coveralls commented May 28, 2019 •

edited

Loading

cmnbroad Jun 7, 2019 •

edited

Loading