Create a new tool, FilterReadsByFlowCellLocation for filtering out reads with specific flow cell coordinates #1992

fleharty · 2025-02-05T15:11:22Z

Filter out reads with specific flow cell coordinates

Description

This pull request adds a new Picard tool—FilterReadsByFlowCellLocation—which is designed to filter out reads from SAM/BAM files based on their flow cell coordinates. The tool parses the read names (assuming an Illumina read name format) and extracts the X and Y coordinates from the last two fields. If a read’s coordinates match the default values (X_COORD = 1000 and Y_COORD = 1000), the read is omitted from the output file.

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 github actions

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

…s with specific flow cell locations

kachulis

generally looks good, just a few small questions and potential to refactor the tests.

kachulis · 2025-02-05T17:21:28Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+        // Example format: @HWUSI-EAS100R:6:73:941:1973#0/1
+        // or: @EAS139:136:FC706VJ:2:2104:15343:197393
+        try {
+            String[] parts = readName.split(":");


any reason to parse the reads yourself instead of using ReadNameParser?

kachulis · 2025-02-05T17:34:13Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+                // Write read to output if it doesn't match filter criteria
+                writer.addAlignment(read);
+            }
+        } finally {


use try-with-resources instead.

kachulis · 2025-02-05T17:39:28Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+     * @throws IOException if an I/O error occurs.
+     */
+    private File createSamFile(String[] readNames) throws IOException {
+        File tmpSam = File.createTempFile("FilterFlowCellEdgeReadsTest_input", ".sam");


kachulis · 2025-02-05T17:39:46Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+        tmpSam.deleteOnExit();
+
+        // Create a minimal SAM file header.
+        SAMFileHeader header = new SAMFileHeader();


kachulis · 2025-02-05T17:40:23Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+     * @return the temporary SAM file.
+     * @throws IOException if an I/O error occurs.
+     */
+    private File createSamFile(String[] readNames) throws IOException {


readNames final

kachulis · 2025-02-05T17:42:01Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+     */
+    private int countRecords(File samFile) throws IOException {
+        int count = 0;
+        try (SamReader reader = SamReaderFactory.makeDefault().open(samFile)) {


kachulis · 2025-02-05T17:42:14Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+    private int countRecords(File samFile) throws IOException {
+        int count = 0;
+        try (SamReader reader = SamReaderFactory.makeDefault().open(samFile)) {
+            for (SAMRecord rec : reader) {


kachulis · 2025-02-05T17:51:53Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+public class FilterReadsByFlowCellLocationTest {
+
+    // Temporary files for input and output.
+    private File inputSam;


why share these fields across tests? You're just regenerating them in each test

kachulis · 2025-02-05T17:52:42Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+    }
+
+    @AfterMethod
+    public void tearDown() {


don't think you need this, because I think you can keep the inputSam and outputSam objects local to the tests instead of as class fields

kachulis · 2025-02-05T17:53:20Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+     * – One read with non-matching coordinates ("2000:2000") that should be retained.
+     */
+    @Test
+    public void testMixedReads() throws IOException {


Could you easily refactor this into 1 test and a DataProvider?

lbergelson

This is fine overall, I'm kind of skeptical this tool needs to exist. It seems like it should just be a read filter. There are also some awkward AI decisions, especially in the tests. I also found what is either a bug or an incorrect example of how illumina read names are structured.

lbergelson · 2025-02-05T16:51:17Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+import java.io.IOException;
+
+@CommandLineProgramProperties(
+        summary = "Filters out reads with specific flowcell coordinates",


switch summary / one line summary

lbergelson · 2025-02-05T16:51:30Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+import picard.cmdline.CommandLineProgram;
+import picard.cmdline.StandardOptionDefinitions;
+import picard.cmdline.programgroups.ReadDataManipulationProgramGroup;
+import htsjdk.samtools.util.Log;  // Added this import


unecessary comment

lbergelson · 2025-02-05T16:54:24Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+    public String OUTPUT;
+
+    @Argument(shortName = "X",
+            doc = "X coordinate to filter (default: 1000)",


don't write the default into the comment

you might want to add a minValue annotation to disallow negative numbers

lbergelson · 2025-02-05T16:55:09Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+            optional = true)
+    public int X_COORD = 1000;
+
+    @Argument(shortName = "Y",


lbergelson · 2025-02-05T16:55:22Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+        programGroup = ReadDataManipulationProgramGroup.class
+)
+public class FilterReadsByFlowCellLocation extends CommandLineProgram {
+    // Initialize logger


Useless comment

lbergelson · 2025-02-05T17:06:40Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+
+    private boolean hasFlowcellCoordinates(String readName) {
+        // Parse Illumina read name format
+        // Example format: @HWUSI-EAS100R:6:73:941:1973#0/1


You might want to make use of the ReadNameParser which handles illumina read name -> coordinates, it also has support for abitrary read name schemes as well. (That would require exposing an additional argument though.

lbergelson · 2025-02-05T17:08:32Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+            optional = true)
+    public int Y_COORD = 1000;
+
+    private boolean hasFlowcellCoordinates(String readName) {


I would rename this to "matchesFlowCellCoordinate", make it static, and make it take x and y as inputs. Currently it sounds like it's just checking if the readname includes coordinates, which is different than if it matches the bad coordinate.

lbergelson · 2025-02-05T17:12:32Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+                .referenceSequence(REFERENCE_SEQUENCE)
+                .open(new File(INPUT));
+
+        final SAMFileHeader header = reader.getFileHeader();


You'd be better off declaring these in a try-with-resources header

lbergelson · 2025-02-05T17:13:48Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+import java.io.IOException;
+
+/**
+ * Unit tests for FilterFlowCellEdgeReads using TestNG.


lbergelson · 2025-02-05T18:24:41Z

src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java

+/**
+ * Unit tests for FilterFlowCellEdgeReads using TestNG.
+ */
+public class FilterReadsByFlowCellLocationTest {


We typically create all our files within the test method and don't use setup-tear down. Shared fields like this has makes things complicated in ways we would rather avoid. I believe this is also leaking index files which should ideally be cleaned up.

These tests just aren't idiosyncratic. It would probably be better use dataprovider instead of multiple tests like this.

lbergelson · 2025-02-05T18:26:10Z

Oh, I'm late too the party...

lbergelson · 2025-02-05T18:28:35Z

src/main/java/picard/sam/FilterReadsByFlowCellLocation.java

+
+@CommandLineProgramProperties(
+        summary = "Filters out reads with specific flowcell coordinates",
+        oneLineSummary = "Removes reads from specific flowcell positions from BAM/CRAM files",


Filter out reads from 'a' specific flowcell position. It only does 1.

I would probably add a toplevel comment describing what problem this works around also because it's pretty specific.

yfarjoun · 2025-02-05T18:53:50Z

I wonder if it makes sense to put this as a filter in FilterSamReads.

…

On Wed, Feb 5, 2025 at 10:11 AM Mark Fleharty ***@***.***> wrote: Filter out reads with specific flow cell coordinates Description This pull request adds a new Picard tool—FilterFlowCellEdgeReads—which is designed to filter out reads from SAM/BAM files based on their flow cell coordinates. The tool parses the read names (assuming an Illumina read name format) and extracts the X and Y coordinates from the last two fields. If a read’s coordinates match the default values (X_COORD = 1000 and Y_COORD = 1000), the read is omitted from the output file. 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 github actions 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 ------------------------------ You can view, comment on, or merge this pull request online at: #1992 Commit Summary - 7d6d220 <7d6d220> Create a new tool, FilterFlowCellEdgeReadsTest for filtering out reads with specific flow cell locations File Changes (2 files <https://github.com/broadinstitute/picard/pull/1992/files>) - *A* src/main/java/picard/sam/FilterFlowCellEdgeReads.java <https://github.com/broadinstitute/picard/pull/1992/files#diff-c744530664f2b9e0cc159caa30cb429a355984cc1d5c017e9c1720b39b581d67> (106) - *A* src/test/java/picard/sam/FilterFlowCellEdgeReadsTest.java <https://github.com/broadinstitute/picard/pull/1992/files#diff-7f964701a7769a5ba75506123eb230b6041fbdf4bf094f3528caac2e2335371e> (163) Patch Links: - https://github.com/broadinstitute/picard/pull/1992.patch - https://github.com/broadinstitute/picard/pull/1992.diff — Reply to this email directly, view it on GitHub <#1992>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAU6JUT2QFZQZTK5CREY2TT2OIS3JAVCNFSM6AAAAABWRJ5MX2VHI2DSMVQWIX3LMV43ASLTON2WKOZSHAZTGMRSGQ4TCOI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

fleharty · 2025-02-05T18:55:30Z

@yfarjoun I think that is a very good suggestion

The maximum length of a Java array is not exactly Integer.MAX_VALUE, but slightly less due to the space taken up by the object header. The exact maximum differs depending on the platform and Java version. This was already accounted for in one instance, but not others. This commit fixes the other instances and changes the maximum size in the existing instance to Integer.MAX_VALUE - 32 instead of Integer.MAX_VALUE - 5 to decrease the likelihood of allocation failures on different Java versions and platforms.

lbergelson · 2025-02-11T18:42:37Z

@fleharty You have a test failure now.

gradle suite > Gradle test > picard.sam.FilterReadsByFlowCellLocationTest > testAllReadsFiltered FAILED
    java.lang.AssertionError: No records should be written expected [0] but found [1]
        at org.testng.Assert.fail(Assert.java:110)
        at org.testng.Assert.failNotEquals(Assert.java:1413)
        at org.testng.Assert.assertEqualsImpl(Assert.java:149)
        at org.testng.Assert.assertEquals(Assert.java:131)
        at org.testng.Assert.assertEquals(Assert.java:1240)
        at picard.sam.FilterReadsByFlowCellLocationTest.testAllReadsFiltered(FilterReadsByFlowCellLocationTest.java:161)

Sorry this is more painful than it ideally would be.

Create a new tool, FilterFlowCellEdgeReadsTest for filtering out read…

7d6d220

…s with specific flow cell locations

fleharty changed the title ~~Create a new tool, FilterFlowCellEdgeReadsTest for filtering out read…~~ Create a new tool, FilterFlowCellEdgeReads for filtering out reads with specific flow cell coordinates Feb 5, 2025

Renaming tool to FilterReadsByFlowCellLocation

4a3f36e

fleharty changed the title ~~Create a new tool, FilterFlowCellEdgeReads for filtering out reads with specific flow cell coordinates~~ Create a new tool, FilterReadsByFlowCellLocation for filtering out reads with specific flow cell coordinates Feb 5, 2025

kachulis self-assigned this Feb 5, 2025

kachulis requested changes Feb 5, 2025

View reviewed changes

lbergelson reviewed Feb 5, 2025

View reviewed changes

EthanNelson-Moore and others added 3 commits February 5, 2025 15:54

Updating GH actions upload-artifact v3 -> v4 (#1993)

c3f6a50

Making a few modifications for a PR

28dc615

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a new tool, FilterReadsByFlowCellLocation for filtering out reads with specific flow cell coordinates #1992

Create a new tool, FilterReadsByFlowCellLocation for filtering out reads with specific flow cell coordinates #1992

fleharty commented Feb 5, 2025 •

edited

Loading

kachulis left a comment

kachulis Feb 5, 2025

kachulis Feb 5, 2025

kachulis Feb 5, 2025

kachulis Feb 5, 2025

kachulis Feb 5, 2025

kachulis Feb 5, 2025

kachulis Feb 5, 2025

kachulis Feb 5, 2025

kachulis Feb 5, 2025

kachulis Feb 5, 2025

lbergelson left a comment

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson Feb 5, 2025

lbergelson commented Feb 5, 2025

lbergelson Feb 5, 2025

yfarjoun commented Feb 5, 2025 via email

fleharty commented Feb 5, 2025

lbergelson commented Feb 11, 2025

Create a new tool, FilterReadsByFlowCellLocation for filtering out reads with specific flow cell coordinates #1992

Are you sure you want to change the base?

Create a new tool, FilterReadsByFlowCellLocation for filtering out reads with specific flow cell coordinates #1992

Conversation

fleharty commented Feb 5, 2025 • edited Loading

Description

Checklist (never delete this)

Content

Review

kachulis 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

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

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

lbergelson commented Feb 5, 2025

Choose a reason for hiding this comment

yfarjoun commented Feb 5, 2025 via email

fleharty commented Feb 5, 2025

lbergelson commented Feb 11, 2025

fleharty commented Feb 5, 2025 •

edited

Loading