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.

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

added cellranger vdj/mkvdjref modules, with module test for vdj #3033

Merged
merged 49 commits into from
Apr 28, 2023

Conversation

klkeys
Copy link
Contributor

@klkeys klkeys commented Mar 14, 2023

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 module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
    - [ ] Use BioConda and BioContainers if possible to fulfil software requirements. only works with nfcore/cellranger Docker image

adds new cellranger module cellranger vdj for single cell immune profiling

mostly cribbed from cellranger count and cellranger mkref

includes the corresponding cellranger mkvdj ref just in case

test data included in nf-core/test-datasets#787

@klkeys klkeys self-assigned this Mar 15, 2023
@klkeys klkeys added the new module Adding a new module label Mar 15, 2023
@klkeys klkeys linked an issue Mar 15, 2023 that may be closed by this pull request
4 tasks
@klkeys klkeys marked this pull request as ready for review March 15, 2023 16:55
Copy link
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

Is there a possibility to have a test for mkvdjref?

modules/nf-core/cellranger/mkvdjref/main.nf Outdated Show resolved Hide resolved
modules/nf-core/cellranger/mkvdjref/main.nf Outdated Show resolved Hide resolved
modules/nf-core/cellranger/vdj/main.nf Outdated Show resolved Hide resolved
modules/nf-core/cellranger/vdj/main.nf Outdated Show resolved Hide resolved
@klkeys
Copy link
Contributor Author

klkeys commented Mar 27, 2023

Is there a possibility to have a test for mkvdjref?

@SPPearce regarding a test for mkvdjref, I'm a little torn

cellranger mkvdjref has two possibilities:

  1. creating a ref from a GTF + a FASTA
  2. using a custom fetch-imgt script supplied with cellranger

if I write a mkvdjref test, then I'm inclined to support the 1st but not the second option, since the fetch-imgt script is controlled by 10x

does that sound ok? if so, then I'll draft a mkvdref test 👍🏼

@grst
Copy link
Member

grst commented Mar 28, 2023

  1. Agree on using the cellranger mkvdjref with fasta/gtf. The fetch-imgt script requires an internet connection and in general our workflows should run offline. People can still run it independently and specify it as reference

  2. Is there still any reason to run cellranger vdj over cellranger multi? For paired vdj+gex, cellranger multi is the recommended way. If there's only VDJ, cellranger multi can also be used. That way we would save one additional alignment route in the scrnaseq pipeline:
    image
    (https://support.10xgenomics.com/single-cell-vdj/software/pipelines/latest/using/multi)

@klkeys
Copy link
Contributor Author

klkeys commented Mar 28, 2023

@grst no reason to prefer cellranger vdj over cellranger multi, I started with V(D)J simply because it met a need

but I'll start with cellranger multi once this is done

in case it matters, cellranger multi is different from count and vdj in that it uses a strange CSV-like input format

it will be some work (or maybe unwise to even try) to shoehorn into current nf-core samplesheet standards 😕

flipside is that a cellranger multi module wraps a fundamentally simple command:

    cellranger multi --id ${meta.id} --csv ${meta.cellranger_multi_csv_file}

and then cellranger handles all the other complexities (gene expression, feature barcode, cell multiplexing, V(D)J profiling, etc.)

@grst
Copy link
Member

grst commented Mar 28, 2023

in case it matters, cellranger multi is different from count and vdj in that it uses a strange CSV-like input format it will be some work (or maybe unwise to even try) to shoehorn into current nf-core samplesheet standards confused

I don't see a reason why we can't generate these files on a per-sample basis. In fact, @ErikaKvalem has started to play around with this. It would, of course, require an extension of our samplesheet, but there was some discussion (and a proposed solution) here: nf-core/scrnaseq#174 (comment)

@klkeys
Copy link
Contributor Author

klkeys commented Mar 29, 2023

added requested test for cellranger mkvdjref 👍🏼

@klkeys klkeys requested a review from SPPearce March 29, 2023 02:34
@SPPearce
Copy link
Contributor

Please don't merge just yet, I'm just looking at something here.

@SPPearce
Copy link
Contributor

I have updated the modules to use the prefix variable instead of meta.id for the output. This is set to be meta.id by default, but lets you change the filenames via a config.
I have also removed the --sample $meta.id argument, which the tools are using to specify which fasta when there are multiple inside the folder provided, rather than using all of them. I think this is was likely to cause a failure anytime that the meta.id was set to be anything but the prefix of the fastq filename.
Tests were passing on gitpod with docker, so hopefully still passing here.

Copy link
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

I've made changes with respect to the prefix, hopefully will all pass and then I'm happy.

Copy link
Contributor

@adamrtalbot adamrtalbot left a comment

Choose a reason for hiding this comment

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

Could you use the full URI for the docker images please?

modules/nf-core/cellranger/count/main.nf Outdated Show resolved Hide resolved
modules/nf-core/cellranger/mkvdjref/main.nf Outdated Show resolved Hide resolved
modules/nf-core/cellranger/vdj/main.nf Outdated Show resolved Hide resolved
SPPearce and others added 4 commits April 28, 2023 11:46
Co-authored-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com>
Co-authored-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com>
Co-authored-by: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com>
@SPPearce
Copy link
Contributor

@adamrtalbot , I have just updated to use the full path in all the modules.

@SPPearce
Copy link
Contributor

Right, I'm happy to go now. @adamrtalbot , if that was your only objection I think we are happy to merge?

@SPPearce SPPearce requested a review from jfy133 April 28, 2023 11:27
Copy link
Contributor

@adamrtalbot adamrtalbot left a comment

Choose a reason for hiding this comment

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

apologies for being slow!

@klkeys
Copy link
Contributor Author

klkeys commented Apr 28, 2023

oof this sparked a lot of discussion 😵

thank you for the additional eyes and edits @SPPearce @jfy133 @adamrtalbot

I will proceed with merging now 👍🏼

@klkeys klkeys added this pull request to the merge queue Apr 28, 2023
Merged via the queue into nf-core:master with commit d68b2e6 Apr 28, 2023
@klkeys klkeys deleted the add_cellranger_vdj branch April 28, 2023 16:28
@SPPearce
Copy link
Contributor

Thanks for your hard work and patience @klkeys

@klkeys
Copy link
Contributor Author

klkeys commented Apr 28, 2023

premature celebration? it looks like nf-core linting failed on cellranger/mkref and cellranger/mkvdjref 😣

not sure how this escaped me, but best not dwell on the past, I'll correct this in #3229

@SPPearce
Copy link
Contributor

That's strange, why did the linting tests not run? All the tests that ran were passing.

@klkeys
Copy link
Contributor Author

klkeys commented Apr 28, 2023

no clue 😣

see here: https://github.com/nf-core/modules/actions/runs/4832726781/jobs/8611869687

I've fixed it in #3229

jvfe added a commit to jvfe/modules that referenced this pull request May 2, 2023
* master: (97 commits)
  Use long form docker.io address for Ubuntu (nf-core#3358)
  Default registry to quay.io (nf-core#3344)
  Sbwf impute glimpse2 (nf-core#3349)
  added cellranger vdj/mkvdjref modules, with module test for vdj (nf-core#3033)
  Add space to fix singularity download error on shinyngs modules (nf-core#3351)
  Output channel modification star (nf-core#3350)
  fix corrected typo in output (nf-core#3337)
  give execution permissions to dumpsoftwareversions.py (nf-core#3347)
  fix meta.ymls for dumpsoftware and multiqc (nf-core#3345)
  Fixing failing subworkflow tests (nf-core#3343)
  remove imputeme module since it is evidently no longer supported (nf-core#3223)
  Update path for yaml-schema json (nf-core#3340)
  New module MetaPhlAn4 (nf-core#3298)
  Update ALL bcftools modules (nf-core#3339)
  update docker and singularity version of p7zip (nf-core#3338)
  update sratools/fasterqdump to handle 10X data (nf-core#3336)
  Make MAD plots optional in shinyngs (nf-core#3334)
  reset tools in CI to released version (nf-core#3335)
  new module wisecondorx/gender (nf-core#3333)
  Bump shinyngs modules (nf-core#3323)
  ...
jvfe added a commit to jvfe/modules that referenced this pull request May 2, 2023
* master: (120 commits)
  Use long form docker.io address for Ubuntu (nf-core#3358)
  Default registry to quay.io (nf-core#3344)
  Sbwf impute glimpse2 (nf-core#3349)
  added cellranger vdj/mkvdjref modules, with module test for vdj (nf-core#3033)
  Add space to fix singularity download error on shinyngs modules (nf-core#3351)
  Output channel modification star (nf-core#3350)
  fix corrected typo in output (nf-core#3337)
  give execution permissions to dumpsoftwareversions.py (nf-core#3347)
  fix meta.ymls for dumpsoftware and multiqc (nf-core#3345)
  Fixing failing subworkflow tests (nf-core#3343)
  remove imputeme module since it is evidently no longer supported (nf-core#3223)
  Update path for yaml-schema json (nf-core#3340)
  New module MetaPhlAn4 (nf-core#3298)
  Update ALL bcftools modules (nf-core#3339)
  update docker and singularity version of p7zip (nf-core#3338)
  update sratools/fasterqdump to handle 10X data (nf-core#3336)
  Make MAD plots optional in shinyngs (nf-core#3334)
  reset tools in CI to released version (nf-core#3335)
  new module wisecondorx/gender (nf-core#3333)
  Bump shinyngs modules (nf-core#3323)
  ...
@klkeys klkeys mentioned this pull request Dec 13, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new module: cellranger vdj
5 participants