Auto convert fastx with paraseq #83
Conversation
… of fastx files with a builder method
…d than required by the writer, just take the information necessary
Summary of ChangesHello @noamteyssier, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new utility for converting FASTX files (FASTA/FASTQ) directly into BINSEQ format, leveraging parallel processing for efficiency. It also refines the internal record handling logic within BINSEQ writers to be more robust against records containing extraneous data, while maintaining strictness for missing required data. Furthermore, dependency management has been improved by making certain crates optional, enhancing build flexibility. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable feature for automatically converting FASTX files to BINSEQ format using parallel processing with paraseq. The changes are extensive, including the addition of a new FastxEncoderBuilder, refactoring of record size calculations for better accuracy, and improving validation logic in the writers. The code is well-structured, and the new tests are comprehensive. I've found one critical issue that would prevent compilation and one suggestion for code simplification.
| let (slen, xlen) = if interleaved { | ||
| let mut rset_iter = rset.iter(); | ||
| let Some(Ok(slen)) = rset_iter.next().map(|r| -> Result<usize> { | ||
| let rec = r.map_err(IntoBinseqError::into_binseq_error)?; | ||
| Ok(rec.seq().len()) | ||
| }) else { | ||
| return Err(FastxEncodingError::EmptyFastxFile.into()); | ||
| }; | ||
| let Some(Ok(xlen)) = rset_iter.next().map(|r| -> Result<usize> { | ||
| let rec = r.map_err(IntoBinseqError::into_binseq_error)?; | ||
| Ok(rec.seq().len()) | ||
| }) else { | ||
| return Err(FastxEncodingError::EmptyFastxFile.into()); | ||
| }; | ||
| (slen, xlen) | ||
| } else { | ||
| let mut rset_iter = rset.iter(); | ||
| let Some(Ok(slen)) = rset_iter.next().map(|r| -> Result<usize> { | ||
| let rec = r.map_err(IntoBinseqError::into_binseq_error)?; | ||
| Ok(rec.seq().len()) | ||
| }) else { | ||
| return Err(FastxEncodingError::EmptyFastxFile.into()); | ||
| }; | ||
| (slen, 0) | ||
| }; |
There was a problem hiding this comment.
This function has a compilation error and some code duplication.
-
Compilation Error: The
rvariable in themapclosure is a¶seq::Result<...>, butmap_erris called on it, which takes ownership (self). This will not compile. You need to useas_ref()to work with references inside theResult. -
Code Duplication: The logic for extracting the sequence length from a record is repeated.
Here is a suggested refactoring that fixes the bug and improves maintainability by removing the duplication:
let mut rset_iter = rset.iter();
let get_len = |r: Option<¶seq::Result<paraseq::fastx::RefRecord<'_>>>| -> Result<usize> {
let res = r.ok_or(FastxEncodingError::EmptyFastxFile)?;
res.as_ref()
.map(|rec| rec.seq().len())
.map_err(IntoBinseqError::into_binseq_error)
};
let (slen, xlen) = if interleaved {
(get_len(rset_iter.next())?, get_len(rset_iter.next())?)
} else {
(get_len(rset_iter.next())?, 0)
};| let nucs_per_byte = if matches!(bitsize, BitSize::Two) { | ||
| 4 | ||
| } else { | ||
| 2 | ||
| }; |
There was a problem hiding this comment.
You can simplify this by using the nucs_per_byte() method from the bitnuc::BitSize enum, which makes the code more concise and directly uses the bitnuc crate's API.
let nucs_per_byte = bitsize.nucs_per_byte();
No description provided.