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

Update modules #372

Merged
merged 12 commits into from
Sep 15, 2023
Merged

Update modules #372

merged 12 commits into from
Sep 15, 2023

Conversation

LilyAnderssonLee
Copy link
Contributor

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/taxprofiler 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 Sep 13, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 5a1999c

+| ✅ 156 tests passed       |+
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: 1.1.0
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-09-15 08:35:03

jfy133
jfy133 approved these changes Sep 13, 2023
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

LGTM if tests are passing and we get all the expected output!

Only changes to check is that when converting reference to a channel in the two hostremoval workflows that the reference isn't being consumed after the first sample.

CHANGELOG.md Outdated Show resolved Hide resolved
subworkflows/local/longread_hostremoval.nf Outdated Show resolved Hide resolved
subworkflows/local/shortread_hostremoval.nf Outdated Show resolved Hide resolved
@LilyAnderssonLee LilyAnderssonLee mentioned this pull request Sep 14, 2023
Merged
9 tasks
LilyAnderssonLee and others added 4 commits September 14, 2023 09:18
@LilyAnderssonLee
Simplified the mapping operation.
59af4ba
@LilyAnderssonLee @jfy133
Update CHANGELOG.md
d8f7896 
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
@LilyAnderssonLee @jfy133
Update subworkflows/local/longread_hostremoval.nf
076f0e2 
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
@LilyAnderssonLee @jfy133
Update subworkflows/local/shortread_hostremoval.nf
d12e8cb 
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
@LilyAnderssonLee
Copy link
Contributor Author

LGTM if tests are passing and we get all the expected output!

Only changes to check is that when converting reference to a channel in the two hostremoval workflows that the reference isn't being consumed after the first sample.

You are right. Only two samples were processed by SAMTOOLS_STATS by the new code.
reference_channel = Channel.fromPath(reference)
SAMTOOLS_STATS(bam_bai, reference_channel.map { refPath -> [[:], refPath] })

Outputs in the samtools/stats folder:
image

Here are the outputs based on original module.
image

So we still need to fix this.

@jfy133
Copy link
Member

jfy133 commented Sep 15, 2023

@LilyAnderssonLee I just ran the command based on 34960d6

And this works as expected for me? I do not get any error, and all the expected stats output files are there?

What command did you use to run?

 nextflow run ../main.nf -profile singularity,test_noprofiling --outdir ./results --input samplesheet_local.csv --databases database_v1.1_local.csv --run_diamond

@LilyAnderssonLee
Copy link
Contributor Author

Yes, it also worked for me, and the output files were correct. The only concern was the use of underscore. I thought the use of 'underscore' is not so beautiful and try to find an alternative approach instead of using underscores.
The code I used was:
nextflow run ../taxprofiler_dev/main.nf -profile test_noprofiling,singularity --outdir ./test_run_samtools_stats_new

jfy133
jfy133 approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Samtools stats/view channel fixed and independently verified 👍

@LilyAnderssonLee LilyAnderssonLee merged commit 9ac1158 into nf-core:dev Sep 15, 2023
26 checks passed
@LilyAnderssonLee LilyAnderssonLee deleted the update_modules_final branch October 3, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfy133 jfy133 jfy133 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
@LilyAnderssonLee @jfy133