From 233cf9dcafb00e5cba56f6c793ad2465faea96b0 Mon Sep 17 00:00:00 2001 From: Dror Kessler Date: Sun, 13 Oct 2024 10:24:52 +0200 Subject: [PATCH 1/3] FLOW_* parameters must be accompanied by FLOW_MODE --- .../sam/markduplicates/MarkDuplicates.java | 19 ++++++++++++--- .../MarkDuplicatesForFlowHelperTest.java | 23 +++++++++++++++---- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/main/java/picard/sam/markduplicates/MarkDuplicates.java b/src/main/java/picard/sam/markduplicates/MarkDuplicates.java index 669fb8cbca..666569cad8 100644 --- a/src/main/java/picard/sam/markduplicates/MarkDuplicates.java +++ b/src/main/java/picard/sam/markduplicates/MarkDuplicates.java @@ -27,9 +27,7 @@ import htsjdk.samtools.*; import htsjdk.samtools.DuplicateScoringStrategy.ScoringStrategy; import htsjdk.samtools.util.*; -import org.broadinstitute.barclay.argparser.Argument; -import org.broadinstitute.barclay.argparser.ArgumentCollection; -import org.broadinstitute.barclay.argparser.CommandLineProgramProperties; +import org.broadinstitute.barclay.argparser.*; import org.broadinstitute.barclay.help.DocumentedFeature; import picard.PicardException; import picard.cmdline.programgroups.ReadDataManipulationProgramGroup; @@ -263,6 +261,8 @@ protected int doWork() { // use flow based calculation helper? if ( flowBasedArguments.FLOW_MODE ) { calcHelper = new MarkDuplicatesForFlowHelper(this); + } else { + ensureNoFlowParametersSpecified(); } reportMemoryStats("Start of doWork"); @@ -450,6 +450,19 @@ protected int doWork() { return 0; } + // make sure no FLOW__ parameter was set (except for FLOW_MODE) + private void ensureNoFlowParametersSpecified() { + if ( getCommandLineParser() instanceof CommandLineArgumentParser ) { + final CommandLineArgumentParser parser = (CommandLineArgumentParser)getCommandLineParser(); + for (NamedArgumentDefinition def : parser.getNamedArgumentDefinitions()) { + final String name = def.getLongName(); + if (name != "FLOW_MODE" && name.startsWith("FLOW_") && def.getHasBeenSet()) { + throw new PicardException(name + " specified, but no FLOW_MODE"); + } + } + } + } + /** * package-visible for testing */ diff --git a/src/test/java/picard/sam/markduplicates/MarkDuplicatesForFlowHelperTest.java b/src/test/java/picard/sam/markduplicates/MarkDuplicatesForFlowHelperTest.java index 6486c097f2..df8d450d40 100644 --- a/src/test/java/picard/sam/markduplicates/MarkDuplicatesForFlowHelperTest.java +++ b/src/test/java/picard/sam/markduplicates/MarkDuplicatesForFlowHelperTest.java @@ -299,7 +299,16 @@ public void modify(final AbstractMarkDuplicatesCommandLineProgramTester tester) } @Test(dataProvider = "forFlowDataProvider") - public void testForFlowMDCall(final DuplicateScoringStrategy.ScoringStrategy scoringStrategy, final TestRecordInfo[] recInfos, final String[] params, TesterModifier modifier) { + public void testForFlowMDCall_WithFlowMode(final DuplicateScoringStrategy.ScoringStrategy scoringStrategy, final TestRecordInfo[] recInfos, final String[] params, TesterModifier modifier) { + testForFlowMDCall(scoringStrategy, recInfos, params, modifier, true); + } + + @Test(dataProvider = "forFlowDataProvider") + public void testForFlowMDCall_WithoutFlowMode(final DuplicateScoringStrategy.ScoringStrategy scoringStrategy, final TestRecordInfo[] recInfos, final String[] params, TesterModifier modifier) { + testForFlowMDCall(scoringStrategy, recInfos, params, modifier, false); + } + + private void testForFlowMDCall(final DuplicateScoringStrategy.ScoringStrategy scoringStrategy, final TestRecordInfo[] recInfos, final String[] params, TesterModifier modifier, final boolean withFlowMode) { // get tester, build records final AbstractMarkDuplicatesCommandLineProgramTester tester = @@ -328,7 +337,9 @@ public void testForFlowMDCall(final DuplicateScoringStrategy.ScoringStrategy sco } // add parames, set flow order - tester.addArg("FLOW_MODE=true"); + if ( withFlowMode ) { + tester.addArg("FLOW_MODE=true"); + } for ( final String param : params ) { tester.addArg(param); } @@ -339,8 +350,12 @@ public void testForFlowMDCall(final DuplicateScoringStrategy.ScoringStrategy sco modifier.modify(tester); } - // run test - tester.runTest(); + // run test. tests without flow mode should fail + if ( withFlowMode ) { + tester.runTest(); + } else { + Assert.assertThrows(() -> tester.runTest()); + } } @DataProvider(name ="getFlowSumOfBaseQualitiesDataProvider") From a86772c6e78bb7cda3667163aaa17dbb093f1997 Mon Sep 17 00:00:00 2001 From: Dror Kessler Date: Sun, 13 Oct 2024 14:00:43 +0200 Subject: [PATCH 2/3] ensure legacyTest passes --- .../java/picard/sam/markduplicates/MarkDuplicates.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/main/java/picard/sam/markduplicates/MarkDuplicates.java b/src/main/java/picard/sam/markduplicates/MarkDuplicates.java index 666569cad8..f04a718c9f 100644 --- a/src/main/java/picard/sam/markduplicates/MarkDuplicates.java +++ b/src/main/java/picard/sam/markduplicates/MarkDuplicates.java @@ -460,6 +460,15 @@ private void ensureNoFlowParametersSpecified() { throw new PicardException(name + " specified, but no FLOW_MODE"); } } + } else if ( getCommandLineParser() instanceof LegacyCommandLineArgumentParser ) { + final LegacyCommandLineArgumentParser parser = (LegacyCommandLineArgumentParser)getCommandLineParser(); + for ( final String name : parser.getArgv() ) { + if (!name.startsWith("FLOW_MODE") && name.startsWith("FLOW_")) { + throw new PicardException(name + " specified, but no FLOW_MODE"); + } + + } + } } From 8a61a69b8eb99737292a73f651c3bb53471a0e80 Mon Sep 17 00:00:00 2001 From: Dror Kessler Date: Sun, 13 Oct 2024 14:26:30 +0200 Subject: [PATCH 3/3] ensure legacyTest runs --- .../java/picard/sam/markduplicates/MarkDuplicates.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/main/java/picard/sam/markduplicates/MarkDuplicates.java b/src/main/java/picard/sam/markduplicates/MarkDuplicates.java index f04a718c9f..6d94f18604 100644 --- a/src/main/java/picard/sam/markduplicates/MarkDuplicates.java +++ b/src/main/java/picard/sam/markduplicates/MarkDuplicates.java @@ -462,11 +462,12 @@ private void ensureNoFlowParametersSpecified() { } } else if ( getCommandLineParser() instanceof LegacyCommandLineArgumentParser ) { final LegacyCommandLineArgumentParser parser = (LegacyCommandLineArgumentParser)getCommandLineParser(); - for ( final String name : parser.getArgv() ) { - if (!name.startsWith("FLOW_MODE") && name.startsWith("FLOW_")) { - throw new PicardException(name + " specified, but no FLOW_MODE"); + if ( parser.getArgv() != null ) { + for ( final String name : parser.getArgv() ) { + if (!name.startsWith("FLOW_MODE") && name.startsWith("FLOW_")) { + throw new PicardException(name + " specified, but no FLOW_MODE"); + } } - } }