Skip to content
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

Draft: Add feature for sample demultiplexing followed by immune profiling #365

Draft
wants to merge 21 commits into
base: dev
Choose a base branch
from
Draft

Draft: Add feature for sample demultiplexing followed by immune profiling #365

wants to merge 21 commits into from

Conversation

herpov
Copy link

@herpov herpov commented Aug 16, 2024 •
edited
Loading

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/scrnaseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core 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).

Helle Rus Povlsen added 11 commits August 16, 2024 09:08
cp cellrangermulti to cellrangermulti_vdj in SCRNASEQ workflow
5812f72
cp align_cellrangermulti to align_cellrangermulti_vdj in local subwor…
a7a5cd3 
…kflows
separate processes that generate reference files from cellranger multi
06e9932
installed nf-core/module bamtofastq10x
9bafa8d
add null/ to the gitignore list
2e32c2e
add description of demultiplexing combined with immuneprofiling
6072a4c
add bamtofastq10x module with amendments
27a8c16
move reference creation outside the cellranger multi to avoid rerunni…
a1d0170 
…ng. additionally changing input channels
add subworkflow specific for handling sample demultiplexing followed …
170ec07 
…by immune profiling
implement cellranger multi ref and vdj. branch channels to either run…
3b71ff8 
… cellranger multi or cellranger multi+vdj
update publishDir for the two cellranger multi outputs. add publishDi…
0132d9b 
…r to bamtofastq process
@herpov

This comment was marked as resolved.

Sign in to view
herpov
herpov commented Aug 16, 2024
View reviewed changes
conf/modules.config Outdated Show resolved Hide resolved
herpov
herpov commented Aug 16, 2024
View reviewed changes
conf/modules.config Outdated Show resolved Hide resolved
herpov
herpov commented Aug 16, 2024
View reviewed changes
modules/nf-core/bamtofastq10x/main.nf
Copy link
Author

Choose a reason for hiding this comment

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

I have made several changes to this script, but idk if I should make a PR for the module itself?

Copy link
Member

Choose a reason for hiding this comment

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

The changes you made seem pretty specific to your use-case, so it doesn't make sense to update the central module.
It is also not allows to change modules in a pipeline (-> linting error).

The preferred way would be to make it somehow work with the nf-core module unchanged. If this is not possible, you can make a copy of the module in the "local" folder and adapt it as needed.

Copy link
Author

Choose a reason for hiding this comment

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

@grst now the changes are minimal to this module. The rest I could fix by manipulating the output channel. Should I make a PR for bamtofastq or should I still just move this to "local" dir?

Copy link
Member

Choose a reason for hiding this comment

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

Should I make a PR for bamtofastq

Yes please, the changes look like everyone will benefit from them.

herpov
herpov commented Aug 16, 2024
View reviewed changes
modules/nf-core/bamtofastq10x/main.nf
bamtofastq \\
$args \\
$bam \\
$prefix
Copy link
Author

Choose a reason for hiding this comment

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

Changed the output path from ${prefix}.fastq.gz.

bamtofastq generates a directory containing two folders: one for GEX and one for CMO .fastq files.
The two folders are prefixed with the .bam prefix.
All files are automatically prefixed with bamtofastq.

grst
grst reviewed Aug 20, 2024
View reviewed changes
conf/modules.config Outdated Show resolved Hide resolved
conf/modules.config Outdated Show resolved Hide resolved
modules/nf-core/bamtofastq10x/main.nf
Copy link
Member

Choose a reason for hiding this comment

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

The changes you made seem pretty specific to your use-case, so it doesn't make sense to update the central module.
It is also not allows to change modules in a pipeline (-> linting error).

The preferred way would be to make it somehow work with the nf-core module unchanged. If this is not possible, you can make a copy of the module in the "local" folder and adapt it as needed.

subworkflows/local/align_cellrangermulti_idx.nf Outdated
include { CELLRANGER_MKVDJREF } from "../../modules/nf-core/cellranger/mkvdjref/main.nf"

// Define workflow to subset and index a genome region fasta file
workflow CELLRANGER_MULTI_REF {
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure the workflow name and filename match.

Copy link
Author

Choose a reason for hiding this comment

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

Do you accept the new name of the file?
cellrangermulti_ref.nf

subworkflows/local/align_cellrangermulti_idx.nf Outdated Show resolved Hide resolved
modules/nf-core/bamtofastq10x/main.nf Outdated Show resolved Hide resolved
modules/nf-core/bamtofastq10x/main.nf Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 0132d9b

+| ✅ 205 tests passed       |+
#| ❔   4 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-08-20 07:48:34

@grst
Copy link
Member

grst commented Aug 20, 2024

@grst is this a 10x thing or is it an issue of how nextflow stages the files? Cellranger does not require all files to have the same prefix, ie fastq_id. I'd like some guidance to how I can debug this.

How nextflow stages the files you can check by investigating the process work directory. I haven't worked much wich cellranger multi, but cellranger is pretty strict about the filenames. They need to follow the {sample_name}_S{i}_L00{j}_{R1,R2}_001.fastq.gz convention or they won't be found.

@herpov
Copy link
Author

herpov commented Aug 27, 2024

@grst is this a 10x thing or is it an issue of how nextflow stages the files? Cellranger does not require all files to have the same prefix, ie fastq_id. I'd like some guidance to how I can debug this.

How nextflow stages the files you can check by investigating the process work directory. I haven't worked much wich cellranger multi, but cellranger is pretty strict about the filenames. They need to follow the {sample_name}_S{i}_L00{j}_{R1,R2}_001.fastq.gz convention or they won't be found.

I realized the workflow renamed the files according to the GEX sample name, so I had to ensure that the IDs of the VDJ and AB channels were consistent with the demultiplexed GEX IDs.

@herpov
Copy link
Author

herpov commented Sep 4, 2024

I have not tested the pipeline with frna data. Further, I had to exclude the sample containing probe barcodes from my test set, ie: 4PLEX_HUMAN from assets/cellranger_barcodes_samplesheet.csv. I have not further investigated the reason for failure.
I have been testing the workflow with the linked metadata.

When I tried including another dataset which had been hashed with the same cmo as one of the others I ran into this error:

Screenshot 2024-08-30 at 15 26 45

When running the pipeline separately on the dataset which failed, I had no issues. I have not spent more time trying to work around it, because I don't expect it to be an issue in our use case and, probably, it is a rare event - but thought you'd like to know.

samplesheet.csv
fb_reference.csv
cmo.csv
barcodes_samplesheet.csv

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

@grst grst grst 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.
2 participants
@herpov @grst