Removed plain `assert`s and replaced them with ValidationUtil function #1465

yfarjoun · 2020-02-09T12:18:37Z

some asserts remain, but they have been fixed in a different PR.

Description

Give your PR a concise yet descriptive title
Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?
Mention any issues fixed, addressed or otherwise related to this pull request, including issue numbers or hard links for issues in other repos.
You can delete these instructions once you have written your PR description.

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

…ions. - some asserts remain, but they have been fixed in a different PR.

lbergelson · 2020-02-10T21:32:25Z

Yes... asserts are evil.

yfarjoun · 2020-02-11T09:47:06Z

feel free to review this @lbergelson ....

pshapiro4broad · 2020-02-11T14:42:15Z

src/main/java/picard/vcf/GenotypeConcordanceStates.java

@@ -45,7 +47,7 @@
        public static TruthState getHom(final int alleleIdx) {
            if (alleleIdx == 0) return HOM_REF;
            if (alleleIdx == 1) return HOM_VAR1;
-            assert false;
+            ValidationUtils.validateArg(false,"Shouldn't be here.");


In this case, why not throw new PicardException("Invalid allele index: " + alleleIdx);? This doesn't seem like a proper use of validateArg().

@yfarjoun Wait, I lied, you didn't respond to Phil here.

pshapiro4broad · 2020-02-11T14:43:21Z

src/main/java/picard/analysis/CollectWgsMetricsWithNonZeroCoverage.java

@@ -186,7 +187,7 @@ protected WgsMetrics generateWgsMetrics(final IntervalList intervals,

    @Override
    protected WgsMetricsCollector getCollector(final int coverageCap, final IntervalList intervals) {
-        assert(coverageCap == this.collector.coverageCap);
+        ValidationUtils.validateArg(coverageCap == this.collector.coverageCap,()->"coverageCap has to be the same as the internal coverageCap, found " + coverageCap + " and " + this.collector.coverageCap);


Please reformat the changed lines in the IDE. In this case I would expect to see spaces around the -> operator, and the line length seems long to me.

pshapiro4broad · 2020-02-11T14:45:03Z

src/main/java/picard/fastq/BamToBfqWriter.java

- * one or two of them, depending on whether it's a paired-end read.  This relies on the unmapped
- * BAM file having all paired reads together in order.
- */
-public class BamToBfqWriter {


Did you mean to delete this class? This seems unrelated to your other changes, and maybe deserves its own PR.

this seemed to have been due to the rebase...I didn't do it...

lbergelson

@yfarjoun I agree with @pshapiro4broad 👍 after resolving the minor issues.

lbergelson · 2020-02-11T20:19:30Z

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java

@@ -948,7 +949,7 @@ private void doTest(final String[] args, final File metrics, final int expectedR

            } catch (NoSuchFieldException e) {
                e.printStackTrace();
-                assert false;
+                ValidationUtils.validateArg(false,"Shouldn't be here.");


just throw here

lbergelson · 2020-02-11T20:20:07Z

src/main/java/picard/vcf/GenotypeConcordanceStates.java

@@ -56,7 +58,7 @@ public static TruthState getVar(final int allele0idx, final int allele1idx) {
            if (allele0idx == 1 && allele1idx == 2) return HET_VAR1_VAR2;
            if (allele0idx == 2 && allele1idx == 1) return HET_VAR1_VAR2;

-            assert false;
+            ValidationUtils.validateArg(false,"Shouldn't be here.");


Just throw if we shouldn't be here.

yfarjoun · 2020-02-13T11:36:06Z

src/main/java/picard/fastq/BamToBfqWriter.java

+            // adjust to a shorter length iff clipping tag exists
+            Integer trimPoint = rec.getIntegerAttribute(ReservedTagConstants.XT);
+            if (trimPoint != null) {
+                ValidationUtils.validateArg(rec.getReadLength() == seqs.length, () -> "length of read and seqs differ. Found " + rec.getReadLength() + " and '" + seqs.length + ".");


this is the actual change in the file...everything else is due to newline differences.....

yfarjoun · 2020-02-13T11:36:56Z

since I added a commit...I cannot merge without another review....

lbergelson

@yfarjoun Looks good, although something funky is going on with the BamToBfqWriter. I it supposed to be deleted in master?

lbergelson · 2020-02-13T16:48:22Z

src/main/java/picard/vcf/GenotypeConcordanceStates.java

@@ -109,8 +111,7 @@ public static CallState getHom(final int alleleIdx) {
            if (alleleIdx == 2) return HOM_VAR2;
            if (alleleIdx == 3) return HOM_VAR3;

-            assert false;
-            return null;
+            throw new IllegalStateException("Shouldn't be here.");


lbergelson · 2020-02-13T16:48:26Z

src/main/java/picard/vcf/GenotypeConcordanceStates.java

@@ -124,22 +125,22 @@ public static CallState getHet(int allele0idx, int allele1idx) {
                if (allele1idx == 1) return HET_REF_VAR1;
                if (allele1idx == 2) return HET_REF_VAR2;
                if (allele1idx == 3) return HET_REF_VAR3;
-                assert false;
+                ValidationUtils.validateArg(false,"Shouldn't be here.");


lbergelson · 2020-02-13T16:48:30Z

src/main/java/picard/vcf/GenotypeConcordanceStates.java

                return null;
            }

            //HET CASES
            if(allele0idx == 1) {
                if (allele1idx == 2) return HET_VAR1_VAR2;
                if (allele1idx == 3) return HET_VAR1_VAR3;
-                assert false;
+                ValidationUtils.validateArg(false,"Shouldn't be here.");


lbergelson · 2020-02-13T16:48:32Z

src/main/java/picard/vcf/GenotypeConcordanceStates.java

                return null;
            }

            if(allele0idx == 2 && allele1idx == 3) return HET_VAR3_VAR4; //special case not a mistake.
            if(allele0idx == 3 && allele1idx == 4) return HET_VAR3_VAR4;

-            assert false;
+            ValidationUtils.validateArg(false,"Shouldn't be here.");


lbergelson

@yfarjoun Missed a set of comments

yfarjoun · 2020-02-13T18:17:03Z

thanks @lbergelson fixed.

pshapiro4broad

Looks good. Some (maybe) unused imports to clean up.

pshapiro4broad · 2020-02-20T17:07:23Z

src/main/java/picard/vcf/GenotypeConcordanceStates.java

@@ -1,5 +1,7 @@
 package picard.vcf;

+import htsjdk.utils.ValidationUtils;


import is now unused?

pshapiro4broad · 2020-02-20T17:07:49Z

src/test/java/picard/fingerprint/CrosscheckFingerprintsTest.java

@@ -3,6 +3,7 @@
 import htsjdk.samtools.SamReader;
 import htsjdk.samtools.metrics.MetricsFile;
 import htsjdk.samtools.util.IOUtil;
+import htsjdk.utils.ValidationUtils;


Also unused import

yfarjoun requested a review from pshapiro4broad February 9, 2020 12:18

- removed plain asserts and replaced them with ValidationUtil funct…

daead95

…ions. - some asserts remain, but they have been fixed in a different PR.

yfarjoun force-pushed the yf_remove_asserts branch from 8ba675b to daead95 Compare February 10, 2020 20:05

pshapiro4broad requested changes Feb 11, 2020

View reviewed changes

lbergelson requested changes Feb 11, 2020

View reviewed changes

- responding to review comments

433e350

yfarjoun commented Feb 13, 2020

View reviewed changes

lbergelson approved these changes Feb 13, 2020

View reviewed changes

lbergelson reviewed Feb 13, 2020

View reviewed changes

lbergelson requested changes Feb 13, 2020

View reviewed changes

- missed a few changes

e42c92c

lbergelson approved these changes Feb 13, 2020

View reviewed changes

yfarjoun requested a review from pshapiro4broad February 13, 2020 19:45

pshapiro4broad approved these changes Feb 20, 2020

View reviewed changes

Yossi Farjoun added 2 commits February 20, 2020 22:11

Merge branch 'master' into yf_remove_asserts

e905755

- remove unused imports

8a98a61

yfarjoun merged commit d6cb393 into master Feb 20, 2020

yfarjoun deleted the yf_remove_asserts branch February 20, 2020 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed plain `assert`s and replaced them with ValidationUtil function #1465

Removed plain `assert`s and replaced them with ValidationUtil function #1465

yfarjoun commented Feb 9, 2020

lbergelson commented Feb 10, 2020

yfarjoun commented Feb 11, 2020

pshapiro4broad Feb 11, 2020

lbergelson Feb 13, 2020

pshapiro4broad Feb 11, 2020

pshapiro4broad Feb 11, 2020

yfarjoun Feb 13, 2020

lbergelson left a comment

lbergelson Feb 11, 2020

lbergelson Feb 11, 2020

lbergelson Feb 13, 2020

yfarjoun Feb 13, 2020

yfarjoun commented Feb 13, 2020

lbergelson left a comment

lbergelson Feb 13, 2020

lbergelson Feb 13, 2020

lbergelson Feb 13, 2020

lbergelson Feb 13, 2020

lbergelson left a comment

yfarjoun commented Feb 13, 2020

pshapiro4broad left a comment

pshapiro4broad Feb 20, 2020

pshapiro4broad Feb 20, 2020

		@@ -1,5 +1,7 @@
		package picard.vcf;

		import htsjdk.utils.ValidationUtils;

Removed plain asserts and replaced them with ValidationUtil function #1465

Removed plain asserts and replaced them with ValidationUtil function #1465

Conversation

yfarjoun commented Feb 9, 2020

Description

Checklist (never delete this)

Content

Review

lbergelson commented Feb 10, 2020

yfarjoun commented Feb 11, 2020

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

lbergelson 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

yfarjoun commented Feb 13, 2020

lbergelson 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

lbergelson left a comment

Choose a reason for hiding this comment

yfarjoun commented Feb 13, 2020

pshapiro4broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Removed plain `assert`s and replaced them with ValidationUtil function #1465

Removed plain `assert`s and replaced them with ValidationUtil function #1465