Skip to content

Fastp sc #331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
mapo9 wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Fastp sc #331

mapo9 wants to merge 4 commits into from

Conversation

@mapo9
Copy link
Collaborator

@mapo9 mapo9 commented May 7, 2024

This pr adds the fastp module to the raw single cell based subworkflow.

@mapo9 mapo9 requested a review from ggabernet May 7, 2024 12:25
@github-actions
Copy link

github-actions bot commented May 7, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 09d135a

+| ✅ 205 tests passed       |+
#| ❔   7 tests were ignored |#
!| ❗   1 tests had warnings |!
Details

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-05-08 13:14:53

@ggabernet ggabernet changed the base branch from dev to master May 28, 2024 23:54
@ggabernet ggabernet changed the base branch from master to dev May 28, 2024 23:54
@ggabernet ggabernet changed the base branch from dev to master November 26, 2024 02:12
@ggabernet ggabernet changed the base branch from master to dev November 26, 2024 02:12
ggabernet
ggabernet reviewed Nov 26, 2024
View reviewed changes
modules/local/rename_fastq_cellranger.nf Outdated
tuple val(meta_2), path(orig_r1), path(orig_r2)

output:
tuple val(meta), path('*R1_001.fastq.gz'), path('*R2_001.fastq.gz') , emit: reads
Copy link
Member

@ggabernet ggabernet Nov 26, 2024

Choose a reason for hiding this comment

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

The files won't necessarily end with R1_001.fastq.gz unless we force them to be named this way. Would it be a possibility to rename at this step the fastq files so that they have the name that cellranger expects? We have to be careful with different files from the same sample that correspond to different lanes though

Copy link
Collaborator Author

@mapo9 mapo9 Apr 29, 2025

Choose a reason for hiding this comment

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

good point. I removed the R1_001.fastq.gz part.
However, I think since we only allow 10x right now, the user should give the right naming for now. Also considering lane numbering. I dont know how we could catch the line otherwise

modules/local/rename_fastq_cellranger.nf

script:
"""
mv ${R1} fastp_${orig_r1}
Copy link
Member

@ggabernet ggabernet Nov 26, 2024

Choose a reason for hiding this comment

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

Why adding fastp at the beginning of the filenames?

Copy link
Collaborator Author

@mapo9 mapo9 Apr 29, 2025

Choose a reason for hiding this comment

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

just to show that those are the files that were run through fastp. In accordance to the fastp module adding ".fastp.fastq.gz" which is not possible for cellranger

@jonasscheid
Copy link

jonasscheid commented Mar 10, 2025

Hi!
On behalf of nf-core spring cleaning 2025: Is this PR still relevant/actively worked on or outdated and can be closed?

@mapo9
Copy link
Collaborator Author

mapo9 commented Mar 11, 2025 •
edited
Loading

Hi! On behalf of nf-core spring cleaning 2025: Is this PR still relevant/actively worked on or outdated and can be closed?

It's still necessary, just missed Giselas review

@nf-core-bot
Copy link
Member

nf-core-bot commented Apr 29, 2025

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 2.14.1.
Please update your pipeline to the latest version.

For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation.

@mapo9
Copy link
Collaborator Author

mapo9 commented Apr 29, 2025

On the note of this PR. We use fastp quite a lot here, but as far as I can see it requires the fastqs to be gzipped, right?
However, we never check or zip them if not zipped.
So, if someone uses the pipeline with unzipped files, it will fail.
Or am I missing something?

@ggabernet
Copy link
Member

ggabernet commented Sep 26, 2025

@mapo9 how is this PR looking? Is it ready for review?

Regarding your question, I think it's fair to assume that all users will use gzipped fastq files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggabernet ggabernet ggabernet left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mapo9 @jonasscheid @nf-core-bot @ggabernet