Skip to content

Feat/introduce revcomp - #201

Merged
noamteyssier merged 2 commits into
dev-0.5.12from
feat/introduce-revcomp
Jul 23, 2026
Merged

Feat/introduce revcomp#201
noamteyssier merged 2 commits into
dev-0.5.12from
feat/introduce-revcomp

Conversation

@noamteyssier

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new revcomp command to the bqtools CLI, enabling users to reverse complement sequences in BINSEQ files. It also adds a safety check to prevent writing binary BINSEQ data to stdout without the --pipe flag. The review feedback highlights three key areas for improvement: first, a bug in on_thread_complete where thread-local record counts are not added to the global count, leading to underreported totals; second, a performance optimization for the reverse_complement function to process buffers in a single pass; and third, a bug where single-end files are left untouched instead of being reverse complemented when an invalid mate flag is provided.

Comment thread src/commands/revcomp/processor.rs
Comment on lines +12 to +27
fn reverse_complement(buf: &mut [u8]) {
buf.reverse();
for base in buf.iter_mut() {
*base = match *base {
b'A' => b'T',
b'C' => b'G',
b'G' => b'C',
b'T' => b'A',
b'a' => b't',
b'c' => b'g',
b'g' => b'c',
b't' => b'a',
other => other,
};
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation of reverse_complement performs two passes over the buffer: one to reverse the bytes (buf.reverse()) and another to complement them. Since bqtools is designed for high-performance processing, we can optimize this to a single pass by swapping and complementing from both ends of the buffer simultaneously. This reduces memory bandwidth usage and cache misses.

fn reverse_complement(buf: &mut [u8]) {
    let mut i = 0;
    let mut j = buf.len().saturating_sub(1);
    while i < j {
        let left = complement(buf[i]);
        let right = complement(buf[j]);
        buf[i] = right;
        buf[j] = left;
        i += 1;
        j -= 1;
    }
    if i == j && !buf.is_empty() {
        buf[i] = complement(buf[i]);
    }
}

#[inline]
fn complement(base: u8) -> u8 {
    match base {
        b'A' => b'T',
        b'C' => b'G',
        b'G' => b'C',
        b'T' => b'A',
        b'a' => b't',
        b'c' => b'g',
        b'g' => b'c',
        b't' => b'a',
        other => other,
    }
}

Comment on lines +30 to +39
pub fn run(args: &RevcompCommand) -> Result<()> {
let reader = BinseqReader::new(args.input.path())?;
if !reader.is_paired() && args.mate != Mate::Both {
warn!("Ignoring `--mate/-M` flag as only single channel found in file");
}

let builder = get_builder(args)?;
let ohandle = args.output.as_writer()?;
let writer = builder.build(ohandle)?;
let mut processor = RevCompProcessor::new(writer, args.mate)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

When a single-end file is processed and the user specifies a specific mate (e.g., -M 2), a warning is logged stating that the --mate/-M flag is ignored. However, the code still passes args.mate (which is Mate::Two) to RevCompProcessor. In process_record, rc_primary is computed as matches!(self.mate, Mate::One | Mate::Both). Since self.mate is Mate::Two, rc_primary evaluates to false, and the single channel is left untouched (not reverse complemented). This contradicts the warning and results in silent incorrect behavior.

To fix this, we should override the mate variable to Mate::Both when the file is single-end so that the single channel is actually reverse complemented as warned.

pub fn run(args: &RevcompCommand) -> Result<()> {
    let reader = BinseqReader::new(args.input.path())?;
    let mut mate = args.mate;
    if !reader.is_paired() && mate != Mate::Both {
        warn!("Ignoring `--mate/-M` flag as only single channel found in file");
        mate = Mate::Both;
    }

    let builder = get_builder(args)?;
    let ohandle = args.output.as_writer()?;
    let writer = builder.build(ohandle)?;
    let mut processor = RevCompProcessor::new(writer, mate)?;

@noamteyssier
noamteyssier merged commit 2c007f9 into dev-0.5.12 Jul 23, 2026
8 checks passed
@noamteyssier
noamteyssier deleted the feat/introduce-revcomp branch July 23, 2026 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant