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

Issue 348 #419

Merged
merged 5 commits into from
Oct 11, 2024
Merged

Issue 348 #419

merged 5 commits into from
Oct 11, 2024

Conversation

Schmytzi
Copy link
Collaborator

@Schmytzi Schmytzi commented Oct 8, 2024

Closes #348. The input BED file is now passed through to CALL_SVS, which passes it to BEDTOOLS_MERGE for region-based filtering of SVs during merging.

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>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,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).

Copy link

github-actions bot commented Oct 8, 2024

nf-core lint overall result: Passed ✅

Posted for pipeline commit 60d235f

+| ✅ 169 tests passed       |+
#| ❔  20 tests were ignored |#

❔ Tests ignored:

  • files_exist - File is ignored: CODE_OF_CONDUCT.md
  • files_exist - File is ignored: assets/nf-core-nallo_logo_light.png
  • files_exist - File is ignored: docs/README.md
  • files_exist - File is ignored: docs/images/nf-core-nallo_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-nallo_logo_dark.png
  • files_exist - File is ignored: .github/ISSUE_TEMPLATE/config.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: conf/modules.config
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • files_unchanged - File ignored due to lint config: CODE_OF_CONDUCT.md
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File ignored due to lint config: assets/nf-core-nallo_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-nallo_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-nallo_logo_dark.png
  • files_unchanged - File ignored due to lint config: docs/README.md
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore
  • modules_config - modules_config

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-10-08 11:39:42

@Schmytzi Schmytzi marked this pull request as ready for review October 8, 2024 11:38
@Schmytzi Schmytzi requested a review from a team as a code owner October 8, 2024 11:38
Copy link
Collaborator

@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.

Cool! For some subworkflows, I've implemented subworkflow tests (#129). You can see that the CALL_SVS test is now failing, and you need to update the tests to reflect you now have 6 inputs instead of 5. Let me know if you need any help 😊

@Schmytzi
Copy link
Collaborator Author

Schmytzi commented Oct 9, 2024

The new linter in nf-core tools 3.0.0 complains about the schema's draft. Just seems to be the protocol (http vs https). Should I push the fix in this PR or do we go about this another way?

@fellen31
Copy link
Collaborator

fellen31 commented Oct 9, 2024

The new linter in nf-core tools 3.0.0 complains about the schema's draft. Just seems to be the protocol (http vs https). Should I push the fix in this PR or do we go about this another way?

I'll work on merging the 3.0.0 template PR later today. In the meantime, if the change required for it to work is only to change http to https, then you can fix that in this PR :)

@Schmytzi
Copy link
Collaborator Author

Schmytzi commented Oct 9, 2024

I tried doing that but it continued to produce more errors. 😵‍💫 So I'll put this on hold and merge your changes into my branch when you're done

@Schmytzi
Copy link
Collaborator Author

I rebased my branch onto dev without conflicts. So everything should be fine now 🤞

Copy link
Collaborator

@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.

Nice, looks really good! One thing you could change if you want, but I'll approve as it stands.

input[2] = SAMTOOLS_FAIDX.out.fai
input[3] = "sniffles"
input[4] = [[],[]]
input[5] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is okay (and I've done similarly), but I'm not sure the coordinates in this file will overlap with any variants we call in this test (?).

If you wanted to, you could check the variants output in this test without any bed file, and then choose an appropriate region overlapping a variant.

You can "create" a custom bed file like this:
https://github.com/nf-core/modules/blob/d7462e71f9129083ce10c3fe953ed401781e0ebd/modules/nf-core/modkit/pileup/tests/main.nf.test#L110-L112

@fellen31
Copy link
Collaborator

Also the nf-core template version comment / template_version (pull_request_target) check fails after the template update, this is a problem on the nf-core side, so we can just ignore it for now. I'll try to disable it until it's fixed.

@Schmytzi Schmytzi merged commit 1263d72 into genomic-medicine-sweden:dev Oct 11, 2024
14 checks passed
@Schmytzi Schmytzi deleted the issue-348 branch October 11, 2024 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Filter SVs on call regions bed
2 participants