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

Add found_in tag and nf-test to call repeat expansions #445

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

fellen31
Copy link
Collaborator

Closes #391

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

@fellen31 fellen31 force-pushed the add-found-in-trgt branch 6 times, most recently from 9d4989b to 4359d94 Compare October 29, 2024 14:57
@fellen31 fellen31 marked this pull request as ready for review October 29, 2024 15:18
@fellen31 fellen31 requested a review from a team as a code owner October 29, 2024 15:18
Comment on lines +69 to +71
sample_bam = SAMTOOLS_SORT_TRGT.out.bam // channel: [ val(meta), path(bam) ]
sample_bai = SAMTOOLS_INDEX_TRGT.out.bai // channel: [ val(meta), path(bai) ]
Copy link

Choose a reason for hiding this comment

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

If its a sample_bam with its corresponding sample_bai, wouldn't it be better to output them together as one channel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think what I wrote in the previous comment holds true somewhat here as well, some tools that require a bam file do not require a bai (although to be fair most do).

I think the tests get easier to read and write if we can standardise and know that there's only a meta and a file in the output channels from each workflow.

E.g.:

path(workflow.out.project_vcf[0][1])).vcf.variantsMD5,
{ assert process.out.tbi[0][1].endsWith(".tbi") }

vs.

path(workflow.out.project_vcf_tbi[0][0][0])).vcf.variantsMD5,
{ assert process.out.project_vcf_tbi[0][0][1].endsWith(".tbi") }

Especially if there's a mix of output channels with different number of elements (which there will be, because not all file types have an index).

Most nf-core modules output one file per output, so to me it makes sense to try to keep subworkflows the same as well.

Copy link

Choose a reason for hiding this comment

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

I think that in this case it's a bit different because you could end up putting together the wrong bam with the wrong bai but it's a mater of taste. So I will approve the PR :)

Copy link

@Lucpen Lucpen left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +69 to +71
sample_bam = SAMTOOLS_SORT_TRGT.out.bam // channel: [ val(meta), path(bam) ]
sample_bai = SAMTOOLS_INDEX_TRGT.out.bai // channel: [ val(meta), path(bai) ]
Copy link

Choose a reason for hiding this comment

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

I think that in this case it's a bit different because you could end up putting together the wrong bam with the wrong bai but it's a mater of taste. So I will approve the PR :)

@fellen31 fellen31 force-pushed the add-found-in-trgt branch 2 times, most recently from d835081 to 2df51f1 Compare October 30, 2024 09:49
@fellen31 fellen31 merged commit 9000326 into genomic-medicine-sweden:dev Oct 30, 2024
20 checks passed
@fellen31 fellen31 deleted the add-found-in-trgt branch October 30, 2024 10:16
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.

Add FOUND_IN tags to repeat calling
2 participants