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

Adding cellranger-arc (multiome scRNA-seq + scATAC support) #274

Merged
merged 71 commits into from
Dec 22, 2023

Conversation

heylf
Copy link
Contributor

@heylf heylf commented Oct 31, 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 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>).
  • 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).

@github-actions
Copy link

github-actions bot commented Oct 31, 2023

nf-core lint overall result: Passed ✅

Posted for pipeline commit c6152b0

+| ✅ 162 tests passed       |+
#| ❔   2 tests were ignored |#

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.11.1
  • Run at 2023-12-22 07:51:35

@heylf
Copy link
Contributor Author

heylf commented Nov 28, 2023

So I move the config generation into the mkref module as suggested. Further, I added two parameters so the user can provide their own config file for cellrangerarc.

Now I go to the final step and add some documentation.

@heylf
Copy link
Contributor Author

heylf commented Nov 29, 2023

@grst @fmalmeida @onurcanbektas I made some final changes to the subworkflows and modules. I also added some documentation to usage and output that should help. If there are not further comments, then this is finished.

Copy link
Member

@grst grst left a comment

Choose a reason for hiding this comment

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

Fantastic @heylf! Only still I'm still wondering is if we can merge nf-core/modules#3576 and use the central module.

We should also double-check if there weren't any last-minute changes to the local module that still need to be ported to that PR.

docs/usage.md Show resolved Hide resolved
@@ -0,0 +1,73 @@
process CELLRANGERARC_COUNT {
Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look if we can do something about the docker issues.

nextflow_schema.json Outdated Show resolved Hide resolved
@heylf
Copy link
Contributor Author

heylf commented Dec 20, 2023

@grst @fmalmeida I think I am not responsible for the lintin error? Not sure what the issue is.

But modules should now also be in sync with modules in the nf-core/modules.

@grst
Copy link
Member

grst commented Dec 21, 2023

Not sure what the issue is.

Definitely not your fault! There was a very recent template update that's not yet merged into scrnaseq. We can ignore the linting here for now.

@grst
Copy link
Member

grst commented Dec 21, 2023

ok, last thing, I promise:
Now that the module is merged, could you install it with nf-core modules install into the nf-core directory, rather than still having this as local module?

@heylf
Copy link
Contributor Author

heylf commented Dec 21, 2023

dont worr grst. Its done now

Copy link
Member

@grst grst left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for the massive effort, @heylf!

@grst grst merged commit bf280ce into nf-core:dev Dec 22, 2023
14 checks passed
@grst grst mentioned this pull request Jan 2, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants