CollectQualityYieldMetricsFlowSpace tool #1932

dror27 · 2023-12-25T13:42:23Z

CollectQualityYieldMetrics for Ultima flow based read files.

cmnbroad · 2024-03-12T17:22:20Z

From picard triage: It looks like some of the FlowBasedRead code is duplicated with GATK, and is utility code. Is there any chance that the code that is common with GATK can be factored out so there is less duplication ? Let us know if this turns out to be burdensome. Otherwise, is this ready to merge ? @meganshand Can you do a quick review on this ?

ilyasoifer · 2024-03-12T19:41:48Z

From picard triage: It looks like some of the FlowBasedRead code is duplicated with GATK, and is utility code. Is there any chance that the code that is common with GATK can be factored out so there is less duplication ? Let us know if this turns out to be burdensome. Otherwise, is this ready to merge ? @meganshand Can you do a quick review on this ?

@cmnbroad - thanks! You are right about the code duplication. I was actually wondering what is the established flow to prevent duplications between the picard and the GATK. Could you suggest or point me to the example? Regarding the PR - could we wait with the review? There is a related tool in the GATK that I would like to have merged there first.

Thanks!

cmnbroad · 2024-03-12T20:05:48Z

@ilyasoifer The picard jar is included in the GATK build, so anything in picard that is public can be accessed and used in GATK (just search GATK for import picard to see some examples).

So then it becomes a question of what is convenient. Importing and using a class of static utilities into GATK from picard is pretty easy, but sharing something like the FlowBasedRead class is a little harder because in GATK it extends SAMRecordToGATKReadAdapter, which is GATK only.

We just wanted to raise the issue and see if there is any refactoring that can be done to minimize duplication (such as factoring out some common code into static utilities that can live in picard, which could then be accessed by both picard and GATK). Obviously keeping duplicating code means updates have to be made in both projects. On the other hand, if you want to make a change in GATK that requires an update to common code in picard, there is some latency incurred to get those changes into GATK, since we generally only update to a new picard jar in GATK when there is a new picard release. So we'll leave it up to you to strike the right balance. Hope that helps.

cmnbroad · 2024-04-09T17:18:52Z

@meganshand Can you review this now ? It looks like the GATK PR has been merged ?

meganshand

I agree with @cmnbroad above that copying code from GATK should be avoided if possible. Did you look into that and determine that leaving it copied here is the best solution (for FlowBasedRead for example)?

Other than that, a few small comments to update it to match the changes made in the GATK PR. Thanks!

meganshand · 2024-04-10T14:16:46Z

src/main/java/picard/analysis/CollectQualityYieldMetrics.java

@@ -92,9 +92,6 @@ public class CollectQualityYieldMetrics extends SinglePassSamProgram {
            "of bases if there are supplemental alignments in the input file.")
    public boolean INCLUDE_SUPPLEMENTAL_ALIGNMENTS = false;

-    @Argument(doc = "If true, calculates flow-specific READ_LENGTH_AVG_Q metrics.")
-    public boolean FLOW_MODE = false;


This change won't be backwards compatible with the previous version of Picard. That's ok with me since you're probably aware of most Ultima users using this tool, but wanted to note it in case you'd rather throw an error message pointing them to CollectQualityYieldMetricsFlow if they provide FLOW_MODE.

Good advice. Restored FLOW_MODE on CollectQualityYieldMetrics. When set, now throws an exception and points to CollectQualityYieldMetricsFlow

meganshand · 2024-04-10T14:20:20Z

src/main/java/picard/analysis/CollectQualityYieldMetricsFlow.java

+/*
+ * The MIT License
+ *
+ * Copyright (c) 2009 The Broad Institute


2009 -> 2024

meganshand · 2024-04-10T14:41:46Z

src/main/java/picard/analysis/CollectQualityYieldMetricsFlow.java

+            // add up quals, and quals >= 20
+            int flow = 0;
+            for (final int qual : quals) {
+                metrics.Q20_EQUIVALENT_YIELD += qual;


Doesn't this need to be above line 161 in order to include non-PF reads?

Q20_EQUIVALENT_YIELD seems to be superfluous. The new tool discards all non PF reads and contains only PF counters (except for TOTAL_READS). Q20_EQUIVALENT_YIELD removed. Your comment is correct, but now that there is no such counters it is resolved.

meganshand · 2024-04-10T15:48:56Z

src/main/java/picard/analysis/CollectQualityYieldMetricsFlow.java

+        public long PF_READS = 0;
+
+        /**
+         * The average read length of all the reads


Suggested change

* The average read length of all the reads

* The average read length of the PF reads

Actually is this read length? Or mean number of flows?

name changed to MEAN_PF_READ_NUMBER_OF_FLOWS

meganshand · 2024-04-10T15:51:11Z

src/main/java/picard/analysis/CollectQualityYieldMetricsSNVQ.java

+/*
+ * The MIT License
+ *
+ * Copyright (c) 2009 The Broad Institute


Suggested change

* Copyright (c) 2009 The Broad Institute

* Copyright (c) 2024 The Broad Institute

meganshand · 2024-04-10T19:55:03Z

src/main/java/picard/analysis/CollectQualityYieldMetricsSNVQ.java

+            this.metrics = new QualityYieldMetrics(useBQForBaseQualities);
+        }
+
+        public void acceptRecord(final SAMRecord rec, final ReferenceSequence ref) {


Since this is all so similar, would there be any way to fold this into the CollectQualityYieldMetricsFlow tool instead? Or is it truly beneficial to have two tools? You're already making a new Metrics class so you can add more metrics to it and in the case that the CRAM hasn't had the BQ tag added (or whatever tag the user specifies for the qualities) you'd just get these metrics applied over the QUALs. It's fine if you want to push back on this suggestion, it just might make this more streamlined (but that's not worth it if the cost is a worse user experience).

I see your point. Still, from a user experience perspective, the SNVQ is a rather separate and unique feature in its own right, hence the original design of a separate tool.

meganshand · 2024-04-10T19:56:35Z

src/main/java/picard/analysis/CollectQualityYieldMetricsSNVQ.java

+                } else {
+                    // zero base bq values
+                    for ( int i = 0 ; i < tmp.length ; i++ ) {
+                        tmp[i] -= 33;


Same comment here as in GATK to use htsjdk rather than handling the byte conversion here yourself.

meganshand · 2024-04-10T20:04:37Z

src/main/java/picard/cmdline/StandardOptionDefinitions.java

@@ -46,4 +46,5 @@ public class StandardOptionDefinitions {
    public static final String MINIMUM_LOD_SHORT_NAME = "LOD";
    public static final String SORT_ORDER_SHORT_NAME = "SO";
    public static final String USE_ORIGINAL_QUALITIES_SHORT_NAME = "OQ";
+    public static final String USE_BQ_FOR_BASE_QUALITIES_SHORT_NAME = "UBQ";


hopefully you won't need this after updating the tool to take the tag from the user rather than using "BQ"

meganshand · 2024-04-10T20:06:22Z

src/test/java/picard/analysis/CollectQualityYieldMetricsFlowTest.java

+/*
+ * The MIT License
+ *
+ * Copyright (c) 2015 The Broad Institute


2015 -> 2024

meganshand · 2024-04-10T20:07:23Z

src/test/java/picard/analysis/CollectQualityYieldMetricsSNVQTest.java

+
+        final CollectQualityYieldMetricsSNVQ.QualityYieldMetrics metrics = output.getMetrics().get(0);
+        Assert.assertEquals(metrics.TOTAL_READS, 26577);
+        Assert.assertTrue(metrics.PF_READS > 0);


Do you only want to check that these values are greater than 0? Or do you know what these values should be?

Right. Actual numbers added.

ilyasoifer · 2024-04-16T15:06:01Z

@cmnbroad and @meganshand - we agree with your suggestion to remove as much as possible code duplication.
So the plan is:

Move as much as possible code from GATK to Picard.
Wait until release of the updated picard version (update of the dependency in the GATK)
Make a PR into the GATK that removes the duplicated code in the GATK and adds dependency on picard.
Does this sound like a reasonable plan? Or is there a faster way of doing this?

cmnbroad · 2024-04-17T13:00:24Z

Sounds right to me.

dror27 · 2024-04-25T11:01:15Z

Sounds right to me.

comments by @meganshand addressed - thank you. Next I'd like to verify that what flow code we have in this pull request is enough to be able to pull it and other dup come from GATK. This is verification step - to save us from having to do yet another picard PR to enable GATK flow code cleanup.

meganshand · 2024-04-29T18:37:44Z

The changes you made to address my comments look good 👍

Am I right in assuming that you'll be adding more to this PR as you're moving code from GATK to Picard? Are you waiting on me for anything at this point?

dror27 · 2024-04-30T06:50:24Z

The changes you made to address my comments look good 👍

Am I right in assuming that you'll be adding more to this PR as you're moving code from GATK to Picard? Are you waiting on me for anything at this point?

Your assumption is correct. I'm in the process of evaluating additional common flow code to be moved from GATK to Picard.

…ework

dror27 · 2024-04-30T11:12:29Z

@meganshand : I have now committed changes to picard which include most of the core flow code (from GATK). I have also arrived at a local build which builds a modified version of GATK against this branch (so we know it is usable in this sense).

I will be happy accept your comments on these changes.

@ilyasoifer : FYI

…est framework" This reverts commit fd8b16d.

meganshand · 2024-04-30T13:54:46Z

@dror27 This looks good to me. Did you also run GATK tests when you built the modified version? If so then that would give me much more confidence.

@cmnbroad Did you also want to take a quick look at this?

dror27 · 2024-05-02T05:28:30Z

@dror27 This looks good to me. Did you also run GATK tests when you built the modified version? If so then that would give me much more confidence.

@cmnbroad Did you also want to take a quick look at this?

@meganshand Yes. I did run all the flow tests on a branch adapted to this picard using something like:

$ ./gradlew test --tests "Flow"

meganshand · 2024-05-02T13:13:37Z

@cmnbroad I'll wait to merge until you've had a chance to voice anything on the latest changes.

cmnbroad · 2024-05-02T14:30:48Z

I won't get to it anytime soon, so I'd say if you're good with it @meganshand, go for it. Thanks.

dror27 force-pushed the CollectQualityYieldMetricsFlowSpace-dev branch from 4cc0e00 to e17dcc8 Compare January 28, 2024 11:44

dror27 marked this pull request as draft February 12, 2024 10:22

dror27 changed the title ~~CollectQualityYieldMetricsFlowSpace tool (please hold on review)~~ CollectQualityYieldMetricsFlowSpace tool Feb 12, 2024

dror27 and others added 27 commits February 15, 2024 13:53

CollectQualityYieldMetricsFlowSpace dev

29e9052

add flow read params to test

f1b69fe

Minimimal reported error probability now calculated from the read.

9725c73

removed unused flow code

9264fab

remove FLOW_MODE from CollectQualityYieldMatrics

b5a041d

FlowSpace -> Flow (simplify tool name)

20856b9

histograms added

8080de6

histogram update

65e46fa

Cleaner treatment of fillingValue + fixed a bug in parsing T0

f6fc79f

restore test passing

9893f06

Code cleanup + INCLUDE_BQ_HISTOGRAM parameter

9dd738b

MEAN_READ_NAME

e9996ed

remove @hidden

8f4f5f1

Fixed crash on unaligned reads, reads with no bases and a typing error

ecdedfb

ceil -> round

a41401f

PCT_Q20/Q30_FLOWS added

351988e

CollectSNVQualityYieldMetrics tool

da63a41

SNVQ stats

d9579c5

Quals taken from BQ

f542c85

Small refactoring - PCT_Q20_FLOWS -> PCT_PF_Q20_FLOWS etc.

e78c84c

CollectQualityYieldMetricsFlow changes

8ed29f5

CollectQualityYieldMetricsSNVQTest changes

5fada25

Typo fix

d806ff1

zero base BQ values

aaa0c34

Update SeriesStats.java

cd76c23

Cleaner treatment of fillingValue + fixed a bug in parsing T0

14806b8

qual range warnings added

1576d5b

various comments addressed

7bd235b

dror27 marked this pull request as ready for review February 19, 2024 13:50

Rename short name for USE_BQ_FOR_BASE_QUALITIES

780c0da

dror27 requested a review from ilyasoifer March 11, 2024 11:20

meganshand requested review from meganshand and removed request for ilyasoifer April 9, 2024 17:52

meganshand self-assigned this Apr 9, 2024

meganshand suggested changes Apr 10, 2024

View reviewed changes

addressing pull request comments

411a75d

dror27 added 2 commits April 30, 2024 14:08

flow core code moved to picard

59278aa

test (double,double) assertions made compatible with future test fram…

fd8b16d

…ework

dror27 requested a review from meganshand April 30, 2024 11:57

dror27 added 2 commits April 30, 2024 15:33

Revert "test (double,double) assertions made compatible with future t…

bd50c59

…est framework" This reverts commit fd8b16d.

FlowBasedHaplotype removed from picard

e3d0557

meganshand approved these changes May 2, 2024

View reviewed changes

meganshand merged commit cd216a7 into broadinstitute:master May 2, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CollectQualityYieldMetricsFlowSpace tool #1932

CollectQualityYieldMetricsFlowSpace tool #1932

dror27 commented Dec 25, 2023

cmnbroad commented Mar 12, 2024

ilyasoifer commented Mar 12, 2024

cmnbroad commented Mar 12, 2024 •

edited

Loading

cmnbroad commented Apr 9, 2024

meganshand left a comment

meganshand Apr 10, 2024

dror27 Apr 25, 2024

meganshand Apr 10, 2024

meganshand Apr 10, 2024

dror27 Apr 25, 2024

meganshand Apr 10, 2024

meganshand Apr 10, 2024

dror27 Apr 25, 2024

meganshand Apr 10, 2024

meganshand Apr 10, 2024

dror27 Apr 25, 2024

meganshand Apr 10, 2024

meganshand Apr 10, 2024

meganshand Apr 10, 2024

meganshand Apr 10, 2024

dror27 Apr 25, 2024

ilyasoifer commented Apr 16, 2024

cmnbroad commented Apr 17, 2024

dror27 commented Apr 25, 2024

meganshand commented Apr 29, 2024

dror27 commented Apr 30, 2024

dror27 commented Apr 30, 2024 •

edited

Loading

meganshand commented Apr 30, 2024

dror27 commented May 2, 2024

meganshand commented May 2, 2024

cmnbroad commented May 2, 2024

	* The average read length of all the reads
	* The average read length of the PF reads

	* Copyright (c) 2009 The Broad Institute
	* Copyright (c) 2024 The Broad Institute

CollectQualityYieldMetricsFlowSpace tool #1932

CollectQualityYieldMetricsFlowSpace tool #1932

Conversation

dror27 commented Dec 25, 2023

cmnbroad commented Mar 12, 2024

ilyasoifer commented Mar 12, 2024

cmnbroad commented Mar 12, 2024 • edited Loading

cmnbroad commented Apr 9, 2024

meganshand 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

Choose a reason for hiding this comment

ilyasoifer commented Apr 16, 2024

cmnbroad commented Apr 17, 2024

dror27 commented Apr 25, 2024

meganshand commented Apr 29, 2024

dror27 commented Apr 30, 2024

dror27 commented Apr 30, 2024 • edited Loading

meganshand commented Apr 30, 2024

dror27 commented May 2, 2024

meganshand commented May 2, 2024

cmnbroad commented May 2, 2024

cmnbroad commented Mar 12, 2024 •

edited

Loading

dror27 commented Apr 30, 2024 •

edited

Loading