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

Fixes #65 #67

Closed
wants to merge 2 commits into from
Closed

Fixes #65 #67

wants to merge 2 commits into from

Conversation

evancofer
Copy link

This is a minor change to fix #65 . This error is caused whenever UMI deduplication is run. The error is due to the fact that BAM_DEDUP_STATS_SAMTOOLS_UMITOOLS_GENOME and BAM_DEDUP_STATS_SAMTOOLS_UMITOOLS_TRANSCRIPTOME are not included or defined anywhere. They should be included by including BAM_DEDUP_STATS_SAMTOOLS_UMITOOLS from the bam_dedup_stats_samtools_umitools workflow. A working version of this (with minor changes) is already in the nf-core rnaseq pipeline, which served as the template for many of these changes.

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/riboseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nf-test test main.nf.test -profile test,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.
  • README.md is updated (including new tool citations and authors/contributors).

@evancofer
Copy link
Author

I see that the linting check failed, but it seems to run fine locally with nf-core 2.14.1 and nextflow 24.04.4.

@lpantano
Copy link

lpantano commented Oct 9, 2024

@evancofer sorry for the delay, can you add a note in the CHANGELOG.md about this, then I can work on merging, thanks!

@evancofer
Copy link
Author

@lpantano I actually think there are still some bugs in this code. I don't have the bandwidth to fix this though, so you can close it if needed. IIRC I gave up on this, because it ended up being a little involved. I think the easiest way to fix the problem however would be just update the RNA-seq portion of this to use a more recent version of the RNA-seq pipeline that this is based off of. I think it worked fine when I used that.

Copy link
Member

@pinin4fjords pinin4fjords left a comment

Choose a reason for hiding this comment

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

Sorry to miss this earlier.

Maybe at this point we should start by copying wholesale the equivalent unit from rnaseq dev, and try from there. I think we may have had some changes in the meantime.

Comment on lines +219 to +220
ch_transcriptome_bam = BAM_SORT_STATS_SAMTOOLS.out.bam
ch_transcriptome_bai = BAM_SORT_STATS_SAMTOOLS.out.bai
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ch_transcriptome_bam = BAM_SORT_STATS_SAMTOOLS.out.bam
ch_transcriptome_bai = BAM_SORT_STATS_SAMTOOLS.out.bai
ch_transcriptome_sorted_bam = BAM_SORT_STATS_SAMTOOLS.out.bam
ch_transcriptome_sorted_bai = BAM_SORT_STATS_SAMTOOLS.out.bai

rnaseq does this (also rename the input to the function below), so that the unsorted bam is not over-written. Maybe try this first?

@iraiosub
Copy link
Contributor

iraiosub commented Dec 2, 2024

I opened a new PR #75 to solve the UMI issue.

@FelixKrueger
Copy link
Contributor

This has now been addressed (see above), closing.

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.

5 participants