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

added indexcov : finding large INDEL using the BAI index #1613

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

added indexcov : finding large INDEL using the BAI index #1613

wants to merge 22 commits into from
+792 −26

Conversation

lindenb
Copy link

@lindenb lindenb commented Aug 5, 2024

original discussion: https://nfcore.slack.com/archives/C05V9FRJYMV/p1722884937141779?thread_ts=1696933327.769139&cid=C05V9FRJYMV

OK all, here I am one year later. I had the time to look at this code ! Let me remind you: goleft/indexcov can be used to fiind large INDEL using the BAI index.I've just implemented this in sarek and on my side I would be ready to submit a PR but I suppose some things needs to be fixed.

  • i added the nf-core/goleft/indexcov module
  • "indexcov" is now allowed in 'params.tools'
  • the subworkflow is in ./subworkflows/local/bam_variant_calling_indexcov/main.nf
    the documentation as been brefly updated
  • I created a local process used to generate a BAI with no unmapped,duplicate, etc... (./subworkflows/local/bam_variant_calling_indexcov/main.nf)
  • indexcov is only called for the germline worklfow, I don't have any experience with somatic/tumor WGS.
  • test are missing, I don't know what I should write. However, nf-test test tests/ --verbose --profile +singularity works
  • new config file ./conf/modules/indexcov.config

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/sarek branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint): Too many warnings but nothing related to 'indexcov'
  • Ensure the test suite passes (nf-test test tests/ --verbose --profile +docker).
  • 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 (I'm not sure how to set this without a PR hash...)
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 5, 2024

This PR is against the master branch ❌

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @lindenb,

It looks like this pull-request is has been made against the lindenb/sarek master branch.
The master branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master are only allowed if they come from the lindenb/sarek dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

@lindenb lindenb changed the base branch from master to dev August 5, 2024 20:00
@github-actions GitHub Actions
Copy link

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

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 1c3cabd

+| ✅ 201 tests passed       |+
#| ❔  12 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ 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:

  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: conf/modules.config
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/sarek/sarek/.github/workflows/awstest.yml
  • template_strings - template_strings
  • modules_config - modules_config

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-10-04 08:06:40

@lindenb
Copy link
Author

lindenb commented Aug 5, 2024

OK, I merged into dev, let's re-run my tests...

@lindenb
Copy link
Author

lindenb commented Aug 6, 2024

I think it ok now, nevertheless there is still a pre-commit error but I don't understand the error.

@asp8200
Copy link
Contributor

asp8200 commented Aug 6, 2024

I think it ok now, nevertheless there is still a pre-commit error but I don't understand the error.

prettier is complaining about the following files:

docs/output.md
modules.json
README.md
docs/usage.md

@lindenb
Copy link
Author

lindenb commented Aug 6, 2024

@asp8200 Thanks, yes I saw this, but was is the error exactly ?

@asp8200
Copy link
Contributor

asp8200 commented Aug 6, 2024 •
edited
Loading

You can run prettier locally on your branch on those above-mentioned files. However, it might also be possible to have a github-action fix it for you. Let's try...

@nf-core-bot fix linting

@asp8200
Copy link
Contributor

asp8200 commented Aug 6, 2024

@nf-core-bot fix linting

maxulysse
maxulysse reviewed Aug 30, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Aug 30, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Aug 30, 2024
View reviewed changes
modules/local/indexcov/main.nf Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I would put this in samtools/reindex_bam instead of indexcov

@lindenb
Copy link
Author

lindenb commented Aug 30, 2024

@maxulysse thank you for the review Maxime, I'm currently away from my code, I'll try to have a look at this next week.

Q: this should be set for WGS calling only, what would be the best way/place to set this test ?
Q: should I put this test into the somatic workflow too ? where ?

@maxulysse
Copy link
Member

@maxulysse thank you for the review Maxime, I'm currently away from my code, I'll try to have a look at this next week.

No worries, I know days can be busy

Q: this should be set for WGS calling only, what would be the best way/place to set this test ?
Q: should I put this test into the somatic workflow too ? where ?

I think I would just allow for it if not using the params.wes, so maybe a message in case tool is used with params, similarly to what we do with other warning messages.
And then I'd say adding this test when calling the variant_calling subworkflows.

@lindenb
Copy link
Author

lindenb commented Sep 3, 2024

@maxulysse back to you, thanks !

maxulysse
maxulysse reviewed Sep 3, 2024
View reviewed changes
docs/output.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Sep 3, 2024
View reviewed changes
docs/usage.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Sep 3, 2024
View reviewed changes
docs/usage.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Sep 3, 2024
View reviewed changes
nextflow_schema.json Outdated Show resolved Hide resolved
@maxulysse
Copy link
Member

Just some minor comments

lindenb and others added 6 commits September 3, 2024 15:18
@lindenb @maxulysse
Update docs/output.md
04086cd 
Co-authored-by: Maxime U Garcia <[email protected]>
@lindenb @maxulysse
Update README.md
6bf0440 
Co-authored-by: Maxime U Garcia <[email protected]>
@lindenb @maxulysse
Update README.md
6f0db0b 
Co-authored-by: Maxime U Garcia <[email protected]>
@lindenb @maxulysse
Update docs/usage.md
8d69004 
Co-authored-by: Maxime U Garcia <[email protected]>
@lindenb @maxulysse
Update nextflow_schema.json
0aa03a4 
Co-authored-by: Maxime U Garcia <[email protected]>
@lindenb @maxulysse
Update docs/usage.md
1224f98 
Co-authored-by: Maxime U Garcia <[email protected]>
@maxulysse
Copy link
Member

@lindenb you know you can batch commit all suggestions from the Files Changed tab?

@lindenb
Copy link
Author

lindenb commented Sep 3, 2024

@maxulysse I know now :-P

maxulysse
maxulysse reviewed Sep 13, 2024
View reviewed changes
CHANGELOG.md Outdated
@@ -62,6 +62,7 @@ Loametjåhkkå is another one of the main peaks of the Pårte massif.
- [#1502](https://github.com/nf-core/sarek/pull/1502) - export CNVs into VCF format in `bam_variant_calling_cnvkit`
- [#1534](https://github.com/nf-core/sarek/pull/1534), [#1573](https://github.com/nf-core/sarek/pull/1573) - Handling `.fastq.gz.spring` files as input
- [#1593](https://github.com/nf-core/sarek/pull/1593) - Prepare release `3.4.2`
- [#1613](https://github.com/nf-core/sarek/pull/1613) - add indexcov
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this line in the current dev section?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse left review comments

@FriederikeHanssen FriederikeHanssen Awaiting requested review from FriederikeHanssen FriederikeHanssen is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lindenb @asp8200 @maxulysse @nf-core-bot @plu1087