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

Add porechop_abi to the pipeline #511

Merged
merged 10 commits into from
Aug 7, 2024
Merged

Add porechop_abi to the pipeline #511

merged 10 commits into from
Aug 7, 2024

Conversation

LilyAnderssonLee
Copy link
Contributor

Add Porechop_ABI to the pipeline as an alternative for adapter removal in long-read Oxford Nanopore data.

Close the issue #145

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/taxprofiler 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).

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 31, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit ac04c4a

+| ✅ 266 tests passed       |+
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in ci.yml: You can customise CI pipeline run tests as required
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-08-06 09:38:27

@LilyAnderssonLee
Copy link
Contributor Author

@jfy133The log file of Porechop_ABI is almost same as the one from Porechop. I am wondering if we could use the MultiQC/porechop module to process Porechop_ABI log files. If so, could you please add that to the multiqc.yml file? I am not very familiar with that.

@LilyAnderssonLee LilyAnderssonLee requested review from sofstam and jfy133 and removed request for sofstam July 31, 2024 08:53
jfy133
jfy133 approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

On the phone review doesn't find anything major!

docs/output.md Outdated Show resolved Hide resolved
@LilyAnderssonLee
Copy link
Contributor Author

multiqc_report_porechop:
multiqc_report_porechop.html.zip
multiqc_report_porechop_abi:
multiqc_report_porechop_abi.html.zip

jfy133
jfy133 reviewed Aug 1, 2024
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

I had a look a tthe MultiuQC reports - they look good except the file/sample names are dfiferent, the ABI is missing the library/run ID (just sample ID)... do you know why that is?

conf/modules.config Show resolved Hide resolved
@LilyAnderssonLee
Copy link
Contributor Author

LilyAnderssonLee commented Aug 1, 2024 •
edited
Loading

I had a look a tthe MultiuQC reports - they look good except the file/sample names are dfiferent, the ABI is missing the library/run ID (just sample ID)... do you know why that is?

Yah I found the issue. Multiqc/porechop takes the sample name from the fastq.gz file at the beginning of the log file.
Top rows of porechop_abi log file:


�[1m�[4mLoading reads�[0m
ERR3201952.fastq.gz
10,000 reads loaded

Top rows of porechop log file:


�[1m�[4mLoading reads�[0m
ERR3201952_ERR3201952_porechop.fastq.gz
10,000 reads loaded

No wait, I think this fastq.gz file passed from the upstream. I will check the source of the file.

@LilyAnderssonLee
Copy link
Contributor Author

Yeah, the problem is spotted. The porechop module was modified by adding a line to create a soft link to ensure the ID matches the rest of the pipeline based on meta.id rather than the input file name. I did the same for the porechop_abi

Porechop_abi multiqc report:
multiqc_report_new.html.zip

jfy133
jfy133 reviewed Aug 1, 2024
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

OK! Then one last thing, in the multiqc_config yaml unde 'extra_fn_clean_exts:' can you add 'postchop_abi` or whatever the suffix is? That way MultiQC will clean the sample name for us for rendering :)

@LilyAnderssonLee
Update multiqc_config.yml
b6b5fc0 
Add porechop and porechop_abi logs files to extra_fn_clean_exts
@LilyAnderssonLee
Copy link
Contributor Author

OK! Then one last thing, in the multiqc_config yaml unde 'extra_fn_clean_exts:' can you add 'postchop_abi` or whatever the suffix is? That way MultiQC will clean the sample name for us for rendering :)

Updated. Not sure why linting failed here, but passed locally.

extra_fn_clean_exts:
  - "kraken2.report.txt"
  - ".txt"
  - ".settings"
  - ".bbduk"
  - ".unmapped"
  - "_filtered"
  - "porechop.log"
  - "porechop_abi.log"  
  - type: remove
    pattern: "_falco"

@jfy133
Copy link
Member

jfy133 commented Aug 2, 2024

Can yuou send a MultiQC report of with each tool as a final check?

@LilyAnderssonLee
Copy link
Contributor Author

multiqc_report_porechop_abi_new.html.zip
multiqc_report_porechop_new.html.zip

@LilyAnderssonLee
Copy link
Contributor Author

@jfy133 Can we merge this PR now?

jfy133
jfy133 reviewed Aug 6, 2024
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

The two MultiQC reports still have _porechop or _porechop_abi on them... I wonder if we need to drop the .log?

assets/multiqc_config.yml Outdated Show resolved Hide resolved
@LilyAnderssonLee @jfy133
Update assets/multiqc_config.yml
ac04c4a 
Co-authored-by: James A. Fellows Yates <[email protected]>
@jfy133
Copy link
Member

jfy133 commented Aug 7, 2024

Thank you very much @LilyAnderssonLee !

@jfy133 jfy133 merged commit c779e73 into nf-core:bouncy-basenji Aug 7, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfy133 jfy133 jfy133 approved these changes

@sofstam sofstam Awaiting requested review from sofstam

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

Successfully merging this pull request may close these issues.
2 participants
@LilyAnderssonLee @jfy133