Skip to content

Added Fastp module#204

Merged
maxulysse merged 26 commits into
devfrom
fastp_dev
Mar 12, 2026
Merged

Added Fastp module#204
maxulysse merged 26 commits into
devfrom
fastp_dev

Conversation

@agrima2010

@agrima2010 agrima2010 commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/seqinspector branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@agrima2010 agrima2010 changed the base branch from master to dev March 11, 2026 13:14
@github-actions

github-actions Bot commented Mar 11, 2026

Copy link
Copy Markdown

nf-core pipelines lint overall result: Passed ✅

Posted for pipeline commit 1b7510a

+| ✅ 196 tests passed       |+
#| ❔   8 tests were ignored |#
#| ❔   1 tests had warnings |#
Details

❔ Tests ignored:

❔ Tests fixed:

✅ Tests passed:

Run details

  • nf-core/tools version 3.5.2
  • Run at 2026-03-12 13:04:51

@nf-core nf-core deleted a comment from github-actions Bot Mar 11, 2026
Comment thread workflows/seqinspector.nf Outdated
Comment thread workflows/seqinspector.nf Outdated
Comment thread tests/MiSeq.nf.test.snap Outdated
Comment thread tests/MiSeq.nf.test.snap Outdated
Comment thread tests/MiSeq.nf.test.snap Outdated
Comment thread tests/NovaSeq6000.nf.test.snap Outdated
Comment thread tests/NovaSeq6000.nf.test.snap Outdated
Comment thread tests/NovaSeq6000.nf.test.snap Outdated
Comment thread tests/NovaSeq6000.nf.test.snap Outdated
Comment thread tests/NovaSeq6000.nf.test.snap Outdated
Comment thread tests/NovaSeq6000.nf.test.snap Outdated
Comment thread tests/NovaSeq6000.nf.test.snap Outdated
Comment thread tests/NovaSeq6000.nf.test.snap Outdated
Comment thread tests/NovaSeq6000.nf.test.snap Outdated
Comment thread tests/PromethION.nf.test.snap Outdated
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
Comment thread workflows/seqinspector.nf Outdated
Comment thread CHANGELOG.md
Comment thread README.md Outdated
Comment thread workflows/seqinspector.nf Outdated
agrima2010 and others added 2 commits March 11, 2026 16:06
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
@maxulysse

Copy link
Copy Markdown
Member

@nf-core-bot fix linting pretty please 🙏

@maxulysse maxulysse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Loving this

Comment thread tests/.nftignore Outdated
Comment on lines +4 to +5
fastqc/*_fastqc.{html,zip}
fastqc/*_raw_fastqc.{html,zip}
fastqc/*_trimmed_fastqc.{html,zip}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the old line was ok, but I like the added specificity

Comment thread workflows/seqinspector.nf

//
// MODULE: Run FastQC
// MODULE: Run FastQC on subsampled reads

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

They actually only are subsampled if seqtk is selected

@pontushojer

Copy link
Copy Markdown
Contributor

Hi I unfortunately did not have time to join the Hackaton but I think fastp would be a great addition! Just saw this PR and had two small comments for you to consider :)

  1. What is the thinking about adding FastQC on the trimmed reads? Fastp computes QC stats on untrimmed and trimmed reads by default which is also included in the MultiQC report. For me this feels a bit redundant.

  2. Is there a plan to use trimmed FASTQ files besides FastQC. Since this is primarily a QC pipeline I would consider running fastp without trimming (--disable_adapter_trimming --disable_quality_filtering --disable_length_filtering --disable_trim_poly_g) by default, with the option to enable trimming for those who need it.

@agrima2010

Copy link
Copy Markdown
Contributor Author

Hey @pontushojer !! Thanks for your comment. It was very useful.

  1. Yes, you are right about the first comment. Fastp does compute QC stats before and after trimming and publish it in the multiQC report. We have removed the fastqc after trimming now.
  2. For the second comment, fastp does adapter trimming by default. And since fastp is an optional tool, the user will only run it if they want to do trimming of reads. As to my knowledge, we don't have any plans yet to use trimmed FASTQ files for any further use. I don't think I understand the purpose of running fastp without trimming.

@pontushojer

Copy link
Copy Markdown
Contributor

Great!

  1. For the second comment, fastp does adapter trimming by default. And since fastp is an optional tool, the user will only run it if they want to do trimming of reads. As to my knowledge, we don't have any plans yet to use trimmed FASTQ files for any further use. I don't think I understand the purpose of running fastp without trimming.

Yes that is true that you can opt-in to run fastp. My thinking was rather that one could use if FastQC i.e. strictly for QC with no read manipulation. This would probably save some runtime (not sure how much). This could be a later addition as well, if there is interest.

Comment thread workflows/seqinspector.nf Outdated
agrima2010 and others added 2 commits March 12, 2026 13:45
Co-authored-by: Pontus Höjer <pontushojer@gmail.com>
@maxulysse maxulysse merged commit b9ab058 into dev Mar 12, 2026
20 checks passed
@maxulysse maxulysse deleted the fastp_dev branch April 8, 2026 09:12
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.

4 participants