Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions src/main/java/picard/sam/FilterReadsByFlowCellLocation.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
package picard.sam;

import htsjdk.samtools.*;
import org.broadinstitute.barclay.argparser.Argument;
import org.broadinstitute.barclay.argparser.CommandLineProgramProperties;
import picard.cmdline.CommandLineProgram;
import picard.cmdline.StandardOptionDefinitions;
import picard.cmdline.programgroups.ReadDataManipulationProgramGroup;
import htsjdk.samtools.util.Log; // Added this import
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unecessary comment

import java.io.File;
import java.io.IOException;

@CommandLineProgramProperties(
summary = "Filters out reads with specific flowcell coordinates",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch summary / one line summary

oneLineSummary = "Removes reads from specific flowcell positions from BAM/CRAM files",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

programGroup = ReadDataManipulationProgramGroup.class
)
public class FilterReadsByFlowCellLocation extends CommandLineProgram {
// Initialize logger
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless comment

private static final Log logger = Log.getInstance(FilterReadsByFlowCellLocation.class);

@Argument(shortName = StandardOptionDefinitions.INPUT_SHORT_NAME,
doc = "Input BAM/CRAM file")
public String INPUT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer PicardHtsPath for new tools.


@Argument(shortName = StandardOptionDefinitions.OUTPUT_SHORT_NAME,
doc = "Output BAM/CRAM file")
public String OUTPUT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise


@Argument(shortName = "X",
doc = "X coordinate to filter (default: 1000)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't write the default into the comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

optional = true)
public int X_COORD = 1000;

@Argument(shortName = "Y",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

doc = "Y coordinate to filter (default: 1000)",
optional = true)
public int Y_COORD = 1000;

private boolean hasFlowcellCoordinates(String readName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Parse Illumina read name format
// Example format: @HWUSI-EAS100R:6:73:941:1973#0/1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the examples given here don't match the test it does. the first example only has 5 parts, but it requires 6 or more. Which should it be?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// or: @EAS139:136:FC706VJ:2:2104:15343:197393
try {
String[] parts = readName.split(":");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (parts.length >= 6) { // Ensure we have enough parts
// The last two numbers are typically X and Y coordinates
int x = Integer.parseInt(parts[parts.length-2]);
int y = Integer.parseInt(parts[parts.length-1].split("[#/]")[0]); // Remove any trailing /1 or #0

return x == X_COORD && y == Y_COORD;
}
} catch (NumberFormatException | ArrayIndexOutOfBoundsException e) {
// If we can't parse the coordinates, assume it doesn't match
return false;
}
return false;
}

@Override
protected int doWork() {
final SamReader reader = SamReaderFactory.makeDefault()
.referenceSequence(REFERENCE_SEQUENCE)
.open(new File(INPUT));

final SAMFileHeader header = reader.getFileHeader();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

final SAMFileWriter writer = new SAMFileWriterFactory()
.makeWriter(header, true, new File(OUTPUT), REFERENCE_SEQUENCE);

// Process reads
int totalReads = 0;
int filteredReads = 0;

try {
for (final SAMRecord read : reader) {
totalReads++;

// Check if read has the specified flowcell coordinates
if (hasFlowcellCoordinates(read.getReadName())) {
filteredReads++;
continue; // Skip this read
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use try-with-resources instead.

try {
reader.close();
} catch (IOException e) {
logger.error("Error closing input file", e);
}
writer.close();
}

logger.info("Processed " + totalReads + " total reads");
logger.info("Filtered " + filteredReads + " reads at flowcell position " + X_COORD + ":" + Y_COORD);
logger.info("Wrote " + (totalReads - filteredReads) + " reads to output");

return 0;
}

public static void main(String[] args) {
new FilterReadsByFlowCellLocation().instanceMain(args);
}
}
163 changes: 163 additions & 0 deletions src/test/java/picard/sam/FilterReadsByFlowCellLocationTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
package picard.sam;

import htsjdk.samtools.*;
import org.testng.Assert;
import org.testng.annotations.AfterMethod;
import org.testng.annotations.Test;

import java.io.File;
import java.io.IOException;

/**
* Unit tests for FilterFlowCellEdgeReads using TestNG.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless

*/
public class FilterReadsByFlowCellLocationTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Temporary files for input and output.
private File inputSam;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private File outputSam;

/**
* Helper method to create a temporary SAM file with one record per provided read name.
* Each record is given minimal required fields.
*
* @param readNames an array of read names to include.
* @return the temporary SAM file.
* @throws IOException if an I/O error occurs.
*/
private File createSamFile(String[] readNames) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readNames final

File tmpSam = File.createTempFile("FilterFlowCellEdgeReadsTest_input", ".sam");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

tmpSam.deleteOnExit();

// Create a minimal SAM file header.
SAMFileHeader header = new SAMFileHeader();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

header.setSortOrder(SAMFileHeader.SortOrder.unsorted);
// Add one sequence record so that records have a reference.
header.addSequence(new SAMSequenceRecord("chr1", 1000000));

// Use SAMFileWriterFactory to write a SAM file.
try (SAMFileWriter writer = new SAMFileWriterFactory().makeWriter(header, false, tmpSam, null)) {
// Create one record for each read name.
for (String readName : readNames) {
SAMRecord rec = new SAMRecord(header);
rec.setReadName(readName);
rec.setReferenceName("chr1");
rec.setAlignmentStart(1);
rec.setCigarString("50M");
// Set dummy bases and qualities.
rec.setReadString("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
rec.setBaseQualityString("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF");
writer.addAlignment(rec);
}
}
return tmpSam;
}

/**
* Helper method to count the number of SAM records in a given SAM file.
*
* @param samFile the SAM file.
* @return the number of records.
* @throws IOException if an I/O error occurs.
*/
private int countRecords(File samFile) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

samFile final

int count = 0;
try (SamReader reader = SamReaderFactory.makeDefault().open(samFile)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

for (SAMRecord rec : reader) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

count++;
}
}
return count;
}

@AfterMethod
public void tearDown() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (inputSam != null && inputSam.exists()) {
inputSam.delete();
}
if (outputSam != null && outputSam.exists()) {
outputSam.delete();
}
}

/**
* Test with a mixed input:
* – One read with a name that matches the default coordinates ("1000:1000") and should be filtered out.
* – One read with non-matching coordinates ("2000:2000") that should be retained.
*/
@Test
public void testMixedReads() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

String[] readNames = new String[]{
"EAS139:136:FC706VJ:2:1000:1000", // should be filtered out (matches default X_COORD and Y_COORD)
"EAS139:136:FC706VJ:2:2000:2000" // should be retained
};
inputSam = createSamFile(readNames);
outputSam = File.createTempFile("FilterFlowCellEdgeReadsTest_output", ".sam");
outputSam.deleteOnExit();

FilterReadsByFlowCellLocation tool = new FilterReadsByFlowCellLocation();
tool.INPUT = inputSam.getAbsolutePath();
tool.OUTPUT = outputSam.getAbsolutePath();
// Use default X_COORD=1000, Y_COORD=1000

int ret = tool.doWork();
Assert.assertEquals(ret, 0, "doWork() should return 0");

// Only the record that does not match the filter should be written.
int recordCount = countRecords(outputSam);
Assert.assertEquals(recordCount, 1, "Only one record should be written");
}

/**
* Test with a read whose name does not contain colon-delimited coordinates.
* The method hasFlowcellCoordinates should catch the exception and return false,
* so the record should be retained.
*/
@Test
public void testNonConformingReadName() throws IOException {
String[] readNames = new String[]{
"nonconforming_read" // no colon-separated parts → not filtered
};
inputSam = createSamFile(readNames);
outputSam = File.createTempFile("FilterFlowCellEdgeReadsTest_output", ".sam");
outputSam.deleteOnExit();

FilterReadsByFlowCellLocation tool = new FilterReadsByFlowCellLocation();
tool.INPUT = inputSam.getAbsolutePath();
tool.OUTPUT = outputSam.getAbsolutePath();
// Defaults are used.

int ret = tool.doWork();
Assert.assertEquals(ret, 0);

// The read should be retained.
int recordCount = countRecords(outputSam);
Assert.assertEquals(recordCount, 1, "The nonconforming read should be kept");
}

/**
* Test with an input that has only a read with coordinates matching the filter.
* In this case, the tool should filter out the only record and write an empty output.
*/
@Test
public void testAllReadsFiltered() throws IOException {
String[] readNames = new String[]{
"EAS139:136:FC706VJ:2:1000:1000" // matches filter → filtered out
};
inputSam = createSamFile(readNames);
outputSam = File.createTempFile("FilterFlowCellEdgeReadsTest_output", ".sam");
outputSam.deleteOnExit();

FilterReadsByFlowCellLocation tool = new FilterReadsByFlowCellLocation();
tool.INPUT = inputSam.getAbsolutePath();
tool.OUTPUT = outputSam.getAbsolutePath();
// Defaults: X_COORD=1000, Y_COORD=1000

int ret = tool.doWork();
Assert.assertEquals(ret, 0);

// Expect zero records in the output.
int recordCount = countRecords(outputSam);
Assert.assertEquals(recordCount, 0, "No records should be written");
}
}
Loading