Add opt-in Bismark combined-index alignment (--combined_index)#616
Open
FelixKrueger wants to merge 11 commits into
Open
Add opt-in Bismark combined-index alignment (--combined_index)#616FelixKrueger wants to merge 11 commits into
--combined_index)#616FelixKrueger wants to merge 11 commits into
Conversation
Adds params.combined_index (default false). When enabled and methylseq builds the index, genome prep builds the combined CT+GA reference (--combined_genome) and alignment uses --combined_index (directional/PBAT) or --combined_index --combined_index_sequential (non-directional), driven by Bowtie 2/HISAT2 -p instead of --multicore (combined mode rejects --multicore). Default off => the classic per-strand path is unchanged (byte-identical). - modules/nf-core/bismark/align: add -p when --combined_index present; the classic --multicore heuristic is untouched - conf/modules/bismark_genomepreparation.config: --combined_genome (gated) - conf/modules/bismark_align.config: per-library combined flags - nextflow.config + nextflow_schema.json: combined_index param - tests/combined_index.nf.test: new (snapshot pending native-Docker run) Concordance-gated (not byte-identical) vs the classic model for all library types. Requires an index built with --combined_genome; pre-built/iGenomes indexes are not compatible.
Generated on oxy via nf-test -profile test,docker (bismark 3.1.0 container), NXF_SYNTAX_PARSER=v1. Both scenarios PASSED (directional 112s, non-directional 43s). The default suite (tests/default.nf.test) passed against the EXISTING snapshots with no --update, confirming the classic per-strand path is byte-identical (combined-only -p never fires on the default path).
The combined-index -p resource-model change to the vendored bismark/align module is now recorded as modules/nf-core/bismark/align/bismark-align.diff (patch key added to modules.json), so `nf-core pipelines lint` no longer flags it as out-of-sync and a future `nf-core modules update` won't silently revert it. Resolves the code-review HIGH finding. (Delivery is local-patch; the change can be upstreamed to nf-core/modules later and the patch dropped.)
…age) Addresses the verify-phase coverage gaps: - save_reference + assert the built Bisulfite_Genome/Combined/BS_combined.* is published -> proves combined mode actually engaged (not a silent classic fall-through; workflow.success alone can't distinguish the two). - Add PBAT and bismark_hisat combined scenarios (were untested). - Add a loud-fail test: combined_index + a pre-built index lacking the combined reference must fail (verified it emits Bismark's '--combined_genome' guidance). Verified on oxy (docker, bismark 3.1.0): all 5 tests PASS; the 4 success scenarios also pass deterministically against the regenerated snapshot with no --update (concordance-gated but reproducible). PE combined not exercised (SE test data).
…sh (nf-core#615) Verify-phase follow-ups (non-blocking recommendations from the dual code review): - docs/usage.md: add a 'Combined-index alignment' section (what it does, the concordance-gated caveat, the iGenomes/pre-built-index incompatibility). - Launch-time warnings in validateInputParameters(): combined_index with a non-bismark aligner / with --local_alignment / with a pre-built --bismark_index. - nextflow_schema.json: note combined_index is bismark-aligners-only. - Anchor the aligner guard (/^bismark/) in the align + genomeprep configs. - Match the combined-index base flag precisely in the module (--combined_index not followed by _), so it can't substring-match the _sequential/_parallel variants; patch (.diff) updated + reverse-apply-verified against the module.
Member
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 3.5.1. For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation. |
`nf-core modules patch` rewrote modules.json with expanded single-element arrays; prettier (CI's 3.6.2) wants them collapsed. Reformatted with prettier; no semantic change (the bismark/align patch key is retained).
FelixKrueger
enabled auto-merge
July 19, 2026 10:32
The combined_index.nf.test snapshot was frozen with Trim Galore 0.6.10 before nf-core#617 (TG 2.3.0) landed on dev. Regenerated on Nextflow 25.04.0 + docker; all 5 scenarios pass (directional, non-directional, PBAT, HISAT2, loud-fail).
…re#606 merge) nf-core#606 (template v4.0.2, multiqc 1.32->1.34) landed on dev and was merged into this branch, re-staling the combined_index snapshot. Regenerated on Nextflow 25.04.0 + docker against the merged modules (multiqc 1.34 + Trim Galore 2.3.0); all 5 scenarios pass.
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
2 participants
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
Follow-up to #614, implementing the alignment-mode modernization discussed in #615.
Adds an opt-in
--combined_indexparameter (defaultfalse) that runs Bismark 3.1.0's combined CT+GA index alignment instead of the classic per-strand +--multicoremodel:--combined_index; non-directional →--combined_index --combined_index_sequential.-p) instead of the fork-based--multicore(combined mode rejects--multicore).bismark_genome_preparation --combined_genome).bismarkandbismark_hisataligners.Default behaviour is unchanged —
--combined_indexis off by default, so existing runs (including AWS iGenomes / pre-built indexes) are byte-identical to before. Combined-index results are concordance-gated (not byte-identical) to the classic model, so this is intentionally opt-in per the #615 discussion; a future default-flip is deferred until combined indexes can be pre-hosted (e.g. nf-core/references).Changes
params.combined_index+nextflow_schema.jsonentry + adocs/usage.mdsection.conf/modules/bismark_align.config: inject the per-library combined flags (gated).conf/modules/bismark_genomepreparation.config: build the combined reference with--combined_genome(gated).bismark/alignmodule: use-p(Bowtie 2/HISAT2 threads) in combined mode instead of--multicore. Tracked as annf-core modules patch(modules/nf-core/bismark/align/bismark-align.diff); the classic--multicorepath is unchanged.--local_alignment, or a pre-built--bismark_index).tests/combined_index.nf.test— directional, non-directional, PBAT and HISAT2 scenarios, plus a loud-fail case.Compatibility note
Pre-built indexes (AWS iGenomes
--genome, or a--bismark_indexthat lacks aBisulfite_Genome/Combined/directory) are not compatible with--combined_indexand fail with a clear "build it with--combined_genome" message. This is documented, and since the option is off by default, no existing workflow is affected.Testing
Verified with
nf-test -profile test,docker(bismark 3.1.0 container):Bisulfite_Genome/Combined/*reference is present, confirming combined mode actually engaged.--update, confirming the classic per-strand path remains byte-identical.PE combined-index is not exercised here (the pipeline test data is single-end).
Closes #615.