count mixed SE/PE libraries per-batch instead of pinning subread - #452
Draft
NicoDeVeaux wants to merge 13 commits into
Draft
count mixed SE/PE libraries per-batch instead of pinning subread#452NicoDeVeaux wants to merge 13 commits into
NicoDeVeaux wants to merge 13 commits into
Conversation
Bump every nf-core module and subworkflow via `nf-core modules update --all`
/ `nf-core subworkflows update --all` and reconcile the resulting
call-signature changes in first-party code. No intended behaviour change.
- Reconcile call sites in workflows/atacseq.nf and the affected
subworkflows/{local,nf-core}/*.
- Fix the ch_fasta_fai reference channel: add .first() so the genome
reference broadcasts to every sample instead of being consumed once.
- Consolidate singleton reference-channel plumbing.
- Remove the orphaned custom/getchromsizes module, replaced by
SAMTOOLS_FAIDX(..., get_sizes=true).
- Fix a terminal MultiQC hang: MultiQC must not publish to the `versions`
topic because its own input depends on that topic being resolved, which
creates a cycle that leaves the run hanging forever. Emit the version as
a named output instead, matching the nf-core/multiqc convention.
- Replace fragile '2>&1 | tr | sed;sed' version extraction in local modules
with concise single-command forms (still emitting topic: versions).
- Ignore a merge_markers false positive in the bowtie2 test snapshot: SAM
base-quality strings contain runs of '<' that trip the check.
Chromap BAMs carried no @rg header, so Picard 3.4.0 MarkDuplicates aborted with a NullPointerException (SAMRecord.getReadGroup() is null) once the module update bumped Picard from 3.1.1. Update the fastq_align_chromap subworkflow to d92e0f8, which adds PICARD_ADDORREPLACEREADGROUPS and an update_readgroups input, install the picard/addorreplacereadgroups module, enable read-group updating at the call site in workflows/atacseq.nf, and configure the process in conf/modules.config to mirror the existing BWA/Bowtie2 read-group mapping (RGID=meta.id technical id, RGSM=biological sample, RGPL=ILLUMINA, RGLB=meta.id, RGPU=1, RGCN=params.seq_center). The linked bam_sort_stats_samtools dependency tree also advanced samtools 1.23.1 -> 1.24 as part of the subworkflow update.
The module update bumped subread to 2.1.1, which strictly validates library pairing per BAM: it rejects single-end reads when run in paired mode (-p) and rejects paired-end reads when run in single-end mode. Consensus-peak quantification passes a mixed single-end/paired-end cohort to one SUBREAD_FEATURECOUNTS invocation, so 2.1.1 aborts with "No paired-end reads were detected in paired-end read library". Pin subread back to 2.0.1, which auto-detects pairing per BAM in a single invocation and reproduces the read/fragment counts the existing snapshots encode. This keeps the fix free of custom SE/PE-splitting and matrix-merging code. Remove the pin once mixed-library counting is handled explicitly (follow-up). The pin makes the local module diverge from its registry git_sha, so it is recorded as a tracked nf-core module patch (container + environment.yml) for `nf-core modules lint` check_local_copy.
… order The consensus BAM list is assembled from an unordered channel collect, so the featureCounts invocation received its BAMs (and emitted its count columns) in a run- and host-dependent order. The counts were identical, but the output file's byte content - and therefore its snapshot md5 - varied between runs, making the consensus featureCounts snapshot non-reproducible. Sort the BAM list by filename before featureCounts so the column order (and the output file) is deterministic and identical on any x86_64 host, matching CI.
…ates metrics Regenerate all 7 pipeline nf-test snapshots against the updated modules (subread 2.0.1 pin, samtools bumps, chromap read-group path, deterministic consensus featureCounts column order). The previous snapshots were ~2 months stale, from before the module update. Generated on GitHub-hosted ubuntu-latest x86_64 runners via the update-snapshots workflow, so container digests and architecture match CI. Also extend .nftignore: Picard MarkDuplicates metrics headers carry a per-run "Started on: <timestamp>" line, so their md5 is never reproducible. The existing pattern only covered the default filename (*.MarkDuplicates.metrics.txt), but this pipeline renames them via ext.prefix to *.mkD.sorted.metrics.txt (library) and *.mRp.clN.sorted.metrics.txt (replicate). These were the only non-deterministic files left once the consensus featureCounts inputs were sorted.
Merge the nf-core/tools 4.0.3 template output (the same content as nf-core#449) so this branch lints clean against the current template rather than carrying files_unchanged exemptions for the pre-existing drift in .github/PULL_REQUEST_TEMPLATE.md and the two logo PNGs. Keeps the branch-local .nf-core.yml entries (bowtie2 snapshot merge_markers false positive, bamtools filter config defaults) and regenerates ro-crate-metadata.json. Drop this commit if nf-core#449 lands on dev first.
Add entries for the Chromap read-group fix, the subread 2.0.1 pin and the deterministic consensus featureCounts column order. The template 4.0.3 bump is left to nf-core#449.
NicoDeVeaux
marked this pull request as draft
July 26, 2026 02:36
NicoDeVeaux
force-pushed
the
fix/featurecounts-mixed-se-pe
branch
from
July 26, 2026 02:57
8e82195 to
5cb8f16
Compare
|
NicoDeVeaux
force-pushed
the
fix/featurecounts-mixed-se-pe
branch
from
July 26, 2026 03:04
5cb8f16 to
0cc8432
Compare
❌ nf-test failed with latest Nextflow versionNote Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
See the full run for details. |
NicoDeVeaux
force-pushed
the
fix/featurecounts-mixed-se-pe
branch
from
July 28, 2026 09:28
e705d3e to
4910bdf
Compare
seq_center is free text in the schema, so a value containing whitespace was
split by the shell. On the Chromap path this reached Picard
AddOrReplaceReadGroups as `--RGCN Broad Institute`, and Picard took `Broad` as
the value and rejected the remainder:
Illegal argument value: Positional arguments were provided ',Institute}'
but no positional argument is defined for this tool.
Reproduced against the picard 3.4.0 container this PR pins. Quoting the value
makes Picard receive it as a single argument.
Bowtie2 built `--rg CN:${params.seq_center}` the same way and had the same
defect; quoted too. BWA is unaffected because input_check.nf embeds the quotes
in meta.read_group at construction.
No test set seq_center, which is why the matrix stayed green over this. The
Chromap pipeline test now sets a value containing whitespace, so the path is
covered.
The updated subread module publishes *.featureCounts.tsv (and .tsv.summary); the docs still described *.featureCounts.txt. Drop the matching .nftignore patterns rather than renaming them: they are dead, and leaving the .tsv files hashed by the pipeline snapshots is what gives the deterministic column order useful coverage.
Restore subread to the registry version and drop the nf-core patch record. The pin was the stopgap for mixed single-end/paired-end consensus counting; the following commit handles mixed cohorts explicitly, so it is no longer needed. Reverts the pin introduced in nf-core#448.
…ing subread featureCounts (subread >= 2.1.0) applies paired-end mode (-p) per invocation and aborts when one invocation mixes single-end and paired-end BAMs. The consensus BAMs can span both library types, so split them by endedness, count each homogeneous batch with the correct pairing flag, then merge the per-batch matrices into a single consensus table for DESeq2 and MultiQC. The joined tuple arity differs between the two instantiations of this subworkflow: the merged-library caller carries a control-BAM column ([ meta, bam, control, peak ]) while the merged-replicate caller does not ([ meta, bams, peak ]). Index positionally to stay tolerant of both shapes, as the pre-split implementation did. The split replaced the single sorted BAM list from nf-core#448 with two per-endedness batches plus a merge step, each assembled from an unordered channel collect, so all three are sorted by filename. Without that the featureCounts column order, the merged matrix and its snapshot md5 stop being reproducible across runs and hosts — the guarantee nf-core#448 established. Gating is unchanged: the join with ch_peaks keeps only samples that contributed peaks, and combining with MACS3_CONSENSUS.out.saf still requires a consensus to exist (>= 2 samples).
Add unit coverage for the FEATURECOUNTS_MERGE script (per-batch matrices merged into one table, plus the SE/PE fixture pair) and pipeline-level tests for the all-single-end and all-paired-end cohorts, so both homogeneous paths are exercised alongside the mixed default. Each pipeline test snapshots versions.yml, as nf-core lint's nf_test_content rule requires. Snapshots regenerated on GitHub-hosted ubuntu-latest x86_64 runners so container digests and architecture match CI. This also corrects the committed FEATURECOUNTS_MERGE snapshot, which recorded an empty `sed` version because it had been generated on macOS where `sed --version` yields nothing.
The SE/PE split publishes *.SE.featureCounts.tsv and *.PE.featureCounts.tsv alongside the merged matrix; document them and why they exist. Also correct the merged matrix's extension, which the docs still gave as .txt.
NicoDeVeaux
force-pushed
the
fix/featurecounts-mixed-se-pe
branch
from
July 29, 2026 12:08
4910bdf to
90f1773
Compare
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
1 participant
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.
Description
Replaces the temporary
subread2.0.1 pin from #448 with explicit handling of mixedsingle-end/paired-end cohorts, which is the follow-up that PR's commit message flagged:
featureCounts (subread >= 2.1.0) applies paired-end mode (
-p) per invocation and abortswhen one invocation mixes single-end and paired-end BAMs. Consensus quantification now
splits the cohort by endedness, counts each homogeneous batch with the correct pairing
flag, and merges the per-batch matrices into a single consensus table for DESeq2 and
MultiQC.
subreadtherefore tracks the nf-core registry version again, and the pin plusits module patch file are reverted.
Deterministic column ordering from #448 is preserved. The split replaced that PR's single
sorted BAM list with two per-endedness batches plus a merge step, each assembled from an
unordered channel collect, so all three are sorted by filename — otherwise the count
matrix and its snapshot md5 stop being reproducible across runs and hosts.
Stacked on #448 — merge order matters
This must not merge before #448. It reverts that PR's
subreadpin and removes itssubread-featurecounts.diffpatch record; landing first would leavedevwithout the pinwhile still using single-invocation counting.
Until #448 merges, the diff against
devincludes all of it. The delta specific to this PRis 27 files, +1082/−204 (18 files of code and tests, plus regenerated snapshots).
Verification
All 19 checks green at
e705d3e: 14 nf-test shards across Nextflow 25.10.4 andlatest-everything, plus
nf-corelint, pre-commit, and template-version. Snapshots wereregenerated on GitHub-hosted ubuntu-latest x86_64 runners so container digests and
architecture match CI.
Coverage added: a unit test for the
FEATURECOUNTS_MERGEscript (mixed SE+PE column-bindand the single-file pass-through), a pure-bash test of the same script, and pipeline-level
tests for all-single-end and all-paired-end cohorts, so both homogeneous paths are
exercised alongside the mixed default.
MultiQC output is unchanged in shape: it still produces one
multiqc_featureCounts_*tableper consensus level, because the SE and PE summaries cover disjoint BAMs and are merged on
sample name.
The
debugprofile emits no warnings introduced here. The ones it does emit arepre-existing or conditional-path: two
operator 'first' is uselessnotices from #448'sch_fasta_fai.first(), and unmatched config selectors for the read-shifting processes andfor
FASTQ_ALIGN_CHROMAP:PICARD_ADDORREPLACEREADGROUPS(unbound because the test profileuses bwa).
PR checklist
FEATURECOUNTS_MERGEis a local module oversednf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>). — run with-preview(config validation, no task execution); no warnings introduced by this PR, and the newFEATURECOUNTS_MERGEconfig selectors bind correctlydocs/usage.mdis updated. — n/a, no parameter or input changesdocs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors). — n/a, no new tools or citations