Skip to content

Run FastQC and restore its citation text - #76

Open
vinisalazar wants to merge 5 commits into
nf-core:devfrom
vinisalazar:feat/run-fastqc
Open

Run FastQC and restore its citation text#76
vinisalazar wants to merge 5 commits into
nf-core:devfrom
vinisalazar:feat/run-fastqc

Conversation

@vinisalazar

Copy link
Copy Markdown
Collaborator

Module wasn't being called at all

The module was installed, configured and documented in output.md, but no
subworkflow ever called it, so results/fastqc/ was never produced and the
MultiQC report carried nothing but versions and the run summary.

Wires FASTQC onto the pre-merge reads behind skip_preprocessing_qc, feeds
its zip output to MultiQC, and uncomments the FastQC citation and
bibliography entries that were disabled while the process was dormant.
@vinisalazar
vinisalazar requested a review from nickp60 August 5, 2026 02:48
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit b46d2aa

+| ✅ 210 tests passed       |+
#| ❔   5 tests were ignored |#
!| ❗   3 tests had warnings |!
Details

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: 1.0.0
  • readme - README contains the placeholder zenodo.XXXXXXX. This should be replaced with the zenodo doi (after the first release).
  • pipeline_todos - TODO string in CHANGELOG.md: ## v1.0.0 - [unreleased replace with date on release ]

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: .github/workflows/branch.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • files_unchanged - File ignored due to lint config: assets/nf-core-funcprofiler_logo_light.png
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore

✅ Tests passed:

Run details

  • nf-core/tools version 4.1.0
  • Run at 2026-08-05 07:57:16

@vinisalazar vinisalazar mentioned this pull request Aug 5, 2026
11 tasks

@nickp60 nickp60 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @vinisalazar, thanks for getting this started! We had discussed a while ago about not manually calling fastqc, in favor of adding an option to run the nf-core subworkflow fastq_shortread_preprocess_qc. If you feel like we need it for v1, could you swap fastqc for that subworkflow?

A lazy efficient way to get this through for review could be to document how to run the subworkflow directly in the README with nextflow module run nf-core/fastq_shortreads_preprocess_qc syntax, if that can be used with subworkflows (I haven't tried it).

@vinisalazar

Copy link
Copy Markdown
Collaborator Author

Hey Nick, thanks for the refresher. I'll have a go at adding the subworkflow, but if I feel I'm getting stuck, will look into that alternative of documenting that manually

@vinisalazar
vinisalazar marked this pull request as draft August 5, 2026 04:10
DATAPREP now runs FASTQ_SHORTREADS_PREPROCESS_QC per sequencing run, before run
merging, and emits its logs and reports on a new multiqc_files channel. This
replaces the hand-rolled FASTQC call in workflows/funcprofiler.nf.

Adapter trimming, complexity filtering and host removal are exposed through
taxprofiler-style params and are all off by default, so the only behaviour
change out of the box is that FastQC now runs twice, on raw and on processed
reads. The subworkflow's own final concatenation stays off because MERGE_RUNS
already does that job.
@vinisalazar
vinisalazar marked this pull request as ready for review August 5, 2026 07:27
The dev merge kept an allOf entry pointing at preprocessing_run_merging_options
after dev had deleted the group itself, which failed schema_lint on nf-core#76 with
"Subschema `preprocessing_run_merging_options` found in `allOf` but not in
`$defs`". Drop the dangling ref, and the save_runmerged_reads param it described,
which dev had already removed as unused.
The default test disabled FASTQC with ext.when = false, so docs/output.md
advertised a fastqc/ directory the snapshot never contained. tests/.nftignore
already ignored FastQC report contents, so dropping the gate records the paths
without depending on unstable content. Runtime goes from ~170s to ~366s.

Also note that the pipeline never checks db_path against the tool version, with
the combinations exercised on the pilot cohort.
@vinisalazar

Copy link
Copy Markdown
Collaborator Author

Hi @nickp60, sorry this is a big one, as the subworkflow install command added a bunch of files. Is there anything I can do to help with review? Do you know if there's a way to enable the complete test suite?

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.

2 participants