Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for CRAM files as an input format to FastQC, addressing a long-standing feature request (#54). It routes .cram files through the existing BAMFile handler (which uses htsjdk), adds a --reference option to provide a FASTA reference file needed for CRAM decoding, and updates the classpath to include the commons-compress dependency required by the newer htsjdk for CRAM support. It also includes minor Java deprecation fixes in bundled Apache Commons Math source files.
Changes:
- CRAM file format detection, routing, and output filename handling added across
SequenceFactory.java,SequenceFileFilter.java,FastQCConfig.java,OfflineRunner.java, and thefastqcPerl launcher script - New
--reference/-roption added to support providing a FASTA reference file for CRAM decoding inBAMFile.java,FastQCConfig.java, and thefastqcscript - Classpath updated in
run_fastqc.bat,fastqc, and.classpathto includecommons-compress-1.26.0.jar; and Java deprecation warnings fixed in bundledMathUtils.javaandResizableDoubleArray.java
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
uk/ac/babraham/FastQC/Sequence/SequenceFactory.java |
Routes .cram extension and cram/cram_mapped format strings to BAMFile |
uk/ac/babraham/FastQC/Sequence/BAMFile.java |
Passes FASTA reference to SamReaderFactory when decoding CRAM files |
uk/ac/babraham/FastQC/FileFilters/SequenceFileFilter.java |
Adds .cram to the GUI file chooser filter |
uk/ac/babraham/FastQC/FastQCConfig.java |
Adds reference field and validates cram/cram_mapped format strings |
uk/ac/babraham/FastQC/Analysis/OfflineRunner.java |
Strips .cram extension when building the output HTML filename |
fastqc |
Adds --reference CLI option, updates classpath and help text for CRAM |
run_fastqc.bat |
Adds commons-compress-1.26.0.jar to the Windows classpath |
.classpath |
Adds Eclipse classpath entry for commons-compress-1.26.0.jar |
org/apache/commons/math3/util/ResizableDoubleArray.java |
Replaces deprecated new Float().hashCode() with Float.hashCode() |
org/apache/commons/math3/util/MathUtils.java |
Replaces deprecated new Double().hashCode() with Double.hashCode() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #54
Supersedes #176
The leg work was done by @s-andrews and @Pranav-Garg. All I've done here is an additional merge from
masterto make the PR clearer, and a few fixes regarding the JAR/classpath and the file extensions declared (and a couple of Java language fixes).This fixes the bzip2 error I had flagged in #54 (comment). I've also tested all 162 CRAM files from the samtools test suite. The only failures are caused by:
As far as I can see, upgrading htsjdk and supporting recent CRAM specs is as simple as putting a more recent htsjdk in, but the last version compatible with Java 11 is v3.0.5, from 3 years ago. Upgrading to the very latest htsjdk would mean bumping up the minimum Java version to 17. Don't know if that's something we can do, so I haven't committed that change.