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

Pbsv/call #6915

Merged
merged 35 commits into from
Nov 1, 2024
Merged

Pbsv/call #6915

merged 35 commits into from
Nov 1, 2024

Conversation

tanyasarkjain
Copy link
Contributor

@tanyasarkjain tanyasarkjain commented Oct 31, 2024 •
edited
Loading

PR checklist

Closes #XXX

  • 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.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

fellen31
fellen31 reviewed Oct 31, 2024
View reviewed changes
Copy link
Contributor

@fellen31 fellen31 left a comment

Choose a reason for hiding this comment

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

Just a tiny comment!

modules/nf-core/pbsv/call/main.nf Outdated Show resolved Hide resolved
fellen31
fellen31 approved these changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@fellen31 fellen31 left a comment

Choose a reason for hiding this comment

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

Looks good! Consider changing the label or merge as is :)

modules/nf-core/pbsv/call/main.nf Outdated
@@ -0,0 +1,50 @@
process PBSV_CALL {
tag "$meta.id"
label 'process_single'
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this module can use multiple threads consider changing from process_single to process_medium. I'm not sure how much impact this has since I haven't run pbsv myself. Fig.5 in this paper suggests maybe not that much, but they don't report discover and call separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! Might as well do it, thank you!

@tanyasarkjain tanyasarkjain added this pull request to the merge queue Nov 1, 2024
Merged via the queue into nf-core:master with commit baa54ee Nov 1, 2024
16 checks passed
@tanyasarkjain tanyasarkjain deleted the pbsv_call branch November 1, 2024 10:39
@tanyasarkjain tanyasarkjain linked an issue Nov 1, 2024 that may be closed by this pull request
new module: PBSV #6855
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fellen31 fellen31 fellen31 approved these changes

Assignees
No one assigned
Labels
new module Adding a new module Ready for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

new module: PBSV
3 participants
@tanyasarkjain @fellen31 @nf-core-bot