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

Control-FREEC improvements #196

Closed
wants to merge 62 commits into from
Closed

Control-FREEC improvements #196

wants to merge 62 commits into from

Conversation

szilvajuhos
Copy link
Contributor

@szilvajuhos szilvajuhos commented Apr 24, 2020

nf-core/sarek pull request

PR checklist

  • There are multiple issues addressed in this PR: i) adding mappability file Use mappability file in ControlFREEC #191 ii) providing a workaround for a Control-FREEC bug controlFREEC assess_significance.R fails #190 iii) storing pileup files unzipped (as Control-FREEC spends aaages with decompression) iv) adding CLI to provide pre-calculated pileups when running Control-FREEC v) adjusting config file
  • If you've fixed a bug or added code that should be tested, add tests!
  • If necessary, also make a PR on the nf-core/sarek branch on the nf-core/test-datasets repo
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Make sure your code lints (nf-core lint .).
  • Documentation in docs is updated
  • CHANGELOG.md is updated
  • README.md is updated

Learn more about contributing: CONTRIBUTING.md

@szilvajuhos szilvajuhos changed the title Cf Control-FREEC improvements Apr 24, 2020
@szilvajuhos szilvajuhos self-assigned this Apr 24, 2020

if(params.normal_pileup && params.tumor_pileup) // we already have pileups
"""
touch ${prefix}${idSample}.pileup
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand the idea here

Copy link
Member

Choose a reason for hiding this comment

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

So instead of a useless process here and in the next one, I think we make a new channel that match the ControlFreec process with the input mpileups and make it conditional so we don't even use these two processes at all.
That would be better, and safer, and easier to understand

main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
samtools mpileup \
-f ${fasta} ${bam} \
${intervalsOptions} \
| bgzip --threads ${task.cpus} -c > ${prefix}${idSample}.pileup.gz
${intervalsOptions} > ${prefix}${idSample}.pileup # | bgzip --threads ${task.cpus} -c > ${prefix}${idSample}.pileup.gz
Copy link
Member

Choose a reason for hiding this comment

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

if the output is not with the .gz I don't think we're saving it in the results folder actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, have to be fixed in the publishDir as I see

Comment on lines +3758 to +3759

return "--"
Copy link
Member

Choose a reason for hiding this comment

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

no sure why you added that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I could not see the debug messages. Will be restored eventually :)

@maxulysse maxulysse added this to the 2.6 milestone Apr 28, 2020
@maxulysse maxulysse mentioned this pull request May 8, 2020
8 tasks
@maxulysse maxulysse closed this May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants