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

Upgrade simpleaf and alevinqc #361

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Upgrade simpleaf and alevinqc #361

wants to merge 7 commits into from

Conversation

DongzeHE
Copy link
Member

@DongzeHE DongzeHE commented Aug 15, 2024 •
edited by grst
Loading

Close #312

Dear scrnaseq developers (and @rob-p, of course ;P ),

This PR is related to #312. I did the following things:

  1. Update simpleaf version from 0.10.0 to 0.17.2
  2. Update alevinqc version from 1.12.1 to 1.18.0
  3. Add parameter no_piscem in nextflow.config for switching from the default piscem mapper to salmon
  4. Add parameter use_selective_alignment in nextflow.config for switching the mapping strategy from the default pseudo-alignment with structure constraints to selective alignment
  5. Add alevin-fry and simpleaf citations into Citation.md
  6. Replace *_t2g_3col.tsv with *t2g_3col.tsv in SIMPLEAF_INDEX process because the latest simpleaf calls it t2g_3col.tsv, without prefix
  7. Replace the salmon_index parameter with simpleaf_index, because the latest simpleaf can take both prebuilt salmon and piscem indices and salmon_index seems not precise anymore. Please feel free to change this back.

Moreover, alevin-fry and simpleaf, as standalone software tools, are the successors of the alevin module in salmon. In the documentation, I saw that salmon, salmon alevin, alevin-fry, and simpleaf are used interchangeably. Although it doesn't hurt, it might cause confusion because the output directory generated by them have distinct layouts and contents. It might be great if we could update the doc. I can create another PR for updating the documentation if you think this might be useful!!

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).
    Sorry, I don't know what this means.
  • 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>).
    • I encountered a working when using the test profile.
    WARN: There's no process matching config selector: .*:CELLRANGER_COUNT -- Did you mean: CELLRANGER_COUNT?
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
    As I am not sure when this PR will be merged, I leave it here.
  • README.md is updated (including new tool citations and authors/contributors).
  • Does alevin provided filtered output like other aligners do? (cf. Standardize conversion workflow #369)
  • Add gene symbols to h5ad when running Alevin

grst
grst reviewed Aug 15, 2024
View reviewed changes
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.

Thanks for the PR!

I can create another PR for updating the documentation if you think this might be useful!!

That would be fantastic!

modules/local/simpleaf_index.nf
@@ -2,10 +2,10 @@ process SIMPLEAF_INDEX {
tag "$transcript_gtf"
label "process_medium"

conda 'bioconda::simpleaf=0.10.0-1'
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to switch to the central nf-core/modules version at this point (see also #296)?

There are modules for simpleaf_index and simpleaf_quant already, they might also need slight updates though.

Copy link

Choose a reason for hiding this comment

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

The modules look good and switching to that would reduce the maintenance burden of simpleaf for nf-core.

Should I submit a PR there? How should I merge the changes there back to scrnaseq?

Copy link
Member

Choose a reason for hiding this comment

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

yes please! Once the module PRs are merged, you can simply install them with the nf-core tools CLI that you also use for linting:

nf-core modules install

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool! Thanks for the information. I will work on this next week! BTW, the other account, an-altosian, is also me :)

@grst
Copy link
Member

grst commented Aug 15, 2024

Make sure your code lints (nf-core lint).

Sorry, I don't know what this means.

The nf-core CLI provides some linting check that ensures certain quality and consistency criteria are met. The same checks are performed by the CI (see "nf-core linting" check below) and is currently failing because of
image

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 15, 2024 •
edited
Loading

nf-core lint overall result: Failed ❌

Posted for pipeline commit ec7f08e

+| ✅ 203 tests passed       |+
#| ❔   4 tests were ignored |#
!| ❗   3 tests had warnings |!
-| ❌   1 tests failed       |-

❌ Test failures:

  • files_unchanged - docs/images/nf-core-scrnaseq_logo_light.png does not match the template

❗ 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-15 14:17:49

@DongzeHE
Copy link
Member Author

DongzeHE commented Aug 15, 2024 •
edited
Loading

For nf-core CI / aligner: alevin ; NF: 23.04.0 (pull_request), I tested alevinqc locally. It worked fine.

For nf-core linting / pre-commit (pull_request), I ran nf-core lint --fix files_unchanged locally and pushed, it worked fine with no error, but the check still failed. I am not sure how to fix docs/images/nf-core-scrnaseq_logo_light.png does not match the template

@grst
Copy link
Member

grst commented Aug 15, 2024

Let's worry about linting as the last step once you switched to the central nf-core modules.

@DongzeHE DongzeHE mentioned this pull request Aug 26, 2024
Closed
16 tasks
@an-altosian an-altosian mentioned this pull request Sep 16, 2024
Merged
@grst grst mentioned this pull request Oct 28, 2024
Open
19 tasks
This was referenced Nov 7, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@an-altosian an-altosian an-altosian left review comments

@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
scrnaseq
Status: Todo high priority
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DongzeHE @grst @an-altosian