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

add VSEARCH cluster #622

Merged
merged 12 commits into from
Aug 25, 2023
Merged

add VSEARCH cluster #622

merged 12 commits into from
Aug 25, 2023

Conversation

a4000
Copy link
Contributor

@a4000 a4000 commented Aug 18, 2023 •
edited
Loading

Addresses issue: #609
It's not LULU, but I figured VSEARCH cluster would be easier to add for ASV post-clustering because there is already an nf-core module (with biocontainer and bioconda).

I've added a test profile, but I haven't added a .test.snap file yet because I'm not sure what tool I should be using to get the md5 value.

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
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@a4000 a4000 requested a review from d4straub August 18, 2023 07:20
@github-actions
Copy link

github-actions bot commented Aug 18, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 8ec859e

+| ✅ 152 tests passed       |+
#| ❔   3 tests were ignored |#
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • readme - README did not have a Nextflow minimum version badge.
  • schema_lint - Parameter input is not defined in the correct subschema (input_output_options)

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-08-25 01:08:03

d4straub
d4straub reviewed Aug 18, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great!
Could you update the CHANGELOG as well?
I am myself not sure when the clustering should take place, directly after DADA2 ASV generation or rather after filters. Do you know of any advantage/disadvantage for the filter sequence?
About the md5sums, any program should do. But the right way to do it (havent done it myself yet) should be the one explained in slack, i.e. nf-test test --updateSnapshot in the pipeline code clone folder after installing https://github.com/askimed/nf-test

conf/test_vsearchcluster.config Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
docs/output.md Outdated Show resolved Hide resolved
workflows/ampliseq.nf Outdated Show resolved Hide resolved
conf/test_vsearchcluster.config Outdated Show resolved Hide resolved
@d4straub d4straub changed the title Dev add VSEARCH cluster Aug 18, 2023
a4000 and others added 5 commits August 19, 2023 20:36
@a4000
Merge branch 'nf-core:dev' into dev
74467fb
@a4000 @d4straub
Update conf/test_vsearchcluster.config
2b2132e 
Co-authored-by: Daniel Straub <42973691+d4straub@users.noreply.github.com>
@a4000 @d4straub
Update nextflow_schema.json
8a96ce6 
Co-authored-by: Daniel Straub <42973691+d4straub@users.noreply.github.com>
@a4000
followed suggestions made in pull request comments
dbaf504
@a4000
modified to pass lint test
142470f
d4straub
d4straub reviewed Aug 22, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for re-ordering! I found still a few points where to order the code though.
I also tested your branch it it seems fine to me except that it collects as FILTER_CLUSTERS.out.stats asv per sample instead of reads per sample. Please use read count stats in results/overall_summary.tsv (see comment below). edit: sorry, numbers should be identical I think, but still questionable whether ASV counts and read counts should be mixed.

docs/output.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
workflows/ampliseq.nf Show resolved Hide resolved
workflows/ampliseq.nf Outdated Show resolved Hide resolved
workflows/ampliseq.nf Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
workflows/ampliseq.nf Outdated Show resolved Hide resolved
d4straub
d4straub approved these changes Aug 24, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment

Choose a reason for hiding this comment

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

Great, thanks!
Just one more small comment, but I approve already.

tests/pipeline/test.nf.test Outdated Show resolved Hide resolved
a4000 and others added 2 commits August 25, 2023 08:16
@a4000 @d4straub
Update tests/pipeline/test.nf.test
3d9e904 
Co-authored-by: Daniel Straub <42973691+d4straub@users.noreply.github.com>
@a4000
added match vsearch_cluster
8ec859e
@a4000 a4000 merged commit d86c0b1 into nf-core:dev Aug 25, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d4straub d4straub d4straub approved these changes

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
@a4000 @d4straub