8000 Implementation of lofreq callparallel to sarek as variant callers by AitorPeseta · Pull Request #1620 · nf-core/sarek · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Implementation of lofreq callparallel to sarek as variant callers #1620

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
Merged
merged 66 commits into from
Sep 13, 2024
Merged

Implementation of lofreq callparallel to sarek as variant callers #1620

merged 66 commits into from
Sep 13, 2024

Conversation

AitorPeseta
Copy link
Contributor

#1588

  • 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).
  • 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).

We wanted to ask you for a code review in this PR. We are updating Sarek with new modules: lofreq somatic (paired sample variant calling) and lofreq callparallel (tumor-only variant calling). Therefore, there will be two new subworkflows: one for somatic and the other one for tumor-only.

In order to these modules fitting in Sarek, we have made some changes in them in this fork. Consequently, they are not exactly the same modules from nf-core modules repo (they will be updated when we finish this). For this reason, we will be working in parallel to update these modules in nf-core/modules.

To sum up, we wanted to ask for a code review, specifically in these new lofreq subworkflows.

AitorOP and others added 24 commits July 11, 2024 10:12
implementation of lofreq
aeee735
bam_variant_calling_somatic_lofreq
a72e615
Implementation of lofreq somatic and callparallel to Sarek: debug ver…
a21830c 
…sion
New changes in the LoFreq implementation
8a199fd
Set first functional version of lofreq
5df9df4
Implementation of bcftools/filter and new changes to lofreq somatic
d76cd8e
@nevinwu
Update main subworkflows
761927b
call-parallel with bcftools/sort
07d3e06
Remove lofreq somatic
088b5fe
Bedtools and lofreq call parallel with new changes
b2c0771
@nevinwu
Update style, lofreq.config & modules.config
dc6becd
@nevinwu
Merge branch 'dev' into master
35ca8fe
Fix bugs of lofreq somatic and delete lofreq/somatic and bcftools/filter
113c2ba
Commit for the pull
cee9a86
Fix some errors: modules.json, tests of callparallel and main of sarek
e9d1458
@nf-core-bot
[automated] Fix code linting
3af4ec8
Fix error of nf-core lint
aeb6f71
Fix email_template.html
c8b31d2
Concluded my merge
7c921b6
@nf-core-bot
[automated] Fix code linting
dd66b9d
modified: assets/email_template.html
3e445d0
First implementation of lofreq and bedtools
a89513d
modules.json
0584a38
First implementation of lofreq and bedtools
2e70cbd
@AitorPeseta
Copy link
Contributor Author

@nf-core-bot fix linting

@github-actions GitHub Actions
Copy link
github-actions bot commented Aug 12, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit ed2d667

+| ✅ 200 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-09-11 12:25:24

@nevinwu
Copy link
Contributor
nevinwu commented Aug 30, 2024

We have added tests and implemented the use of intervals with lofreq callparallel (which was missing previously). Any further feedback would be apreciated.

maxulysse
maxulysse reviewed Sep 3, 2024
View reviewed changes
maxulysse
maxulysse reviewed Sep 3, 2024
View reviewed changes
@maxulysse
Copy link
Member

Why have the FASTQC png files changed?

Can you update CHANGELOG, I think it's almost good to go

nevinwu and others added 4 commits September 4, 2024 11:53
@nevinwu @maxulysse
Update docs/output.md
f6bd81a 
Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>
@nevinwu @maxulysse
Update docs/output.md
9b435d8 
Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>
@nevinwu
Update changelog
98ae31d
@nevinwu
Merge branch 'dev' into master
af20f19
@nevinwu
Copy link
Contributor
nevinwu commented Sep 4, 2024

Why have the FASTQC png files changed?

To be honest, I don't really know why the FASTQC png files have changed. I think my collegue Àitor unintentionally touched them, git was saying they were changed, so he had to download the ones in master and replace the files in his fork. I don't know if just this change can be reverted or undonde easily.

Can you update CHANGELOG, I think it's almost good to go

CHANGELOG updated, please let me know if there is anything left to be done. :)

@nevinwu
Copy link
Contributor
nevinwu commented Sep 4, 2024

@nf-core-bot fix linting

@nevinwu nevinwu requested a review from maxulysse September 4, 2024 11:51
FriederikeHanssen
FriederikeHanssen previously requested changes Sep 10, 2024
View reviewed changes
Copy link
Contributor
@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

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

Looks really great!!!

One question: What is going on with the main.nf and genemetrics/* files? Why are they changed? The main.nf should at most have punctual inclusion.

One feature request: can you please update the metromap and pipeline overview image?

Other than that ready to merge form my side

@nevinwu
Copy link
Contributor
nevinwu commented Sep 11, 2024

Files that shouln't have been changed should be fixed by now.

I have updated the sarek_workflow image (I have used inkscape if that's okay), but I don't think I have the skills to update the sarek_subway one. Could I get a little help with that please? I am a total newbie to all of this image-editing thing.

@maxulysse
Copy link
Member
maxulysse commented Sep 12, 2024 •
edited
Loading

I'll create an issue, and I'll update the metro map at a later date (before the release).
I noticed some weird changes with the new version of the workflow, but don't worry about it, I'll have a look at the same time.

@maxulysse maxulysse mentioned this pull request Sep 12, 2024
Open
@nevinwu nevinwu dismissed FriederikeHanssen’s stale review September 13, 2024 07:42

metro map with lofreq will be updated before next release (#1651). The other changes have been done.

@nevinwu nevinwu merged commit 59e3fe1 into nf-core:dev Sep 13, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse approved these changes

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

Assignees

@nevinwu nevinwu

@AitorPeseta AitorPeseta

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AitorPeseta @FriederikeHanssen @nevinwu @maxulysse @nf-core-bot
0