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

replace picard/markduplicates module with gatk4/markduplicates #409

Merged
merged 8 commits into from
Oct 23, 2023
Merged

replace picard/markduplicates module with gatk4/markduplicates #409

merged 8 commits into from
Oct 23, 2023

Conversation

rannick
Copy link
Collaborator

@rannick rannick commented Sep 25, 2023 •
edited
Loading

the nf-core module gatk4/markduplicates is slightly different from picard/markduplicates in handling the tmp memory location. As we encountered issues with memory for markduplicates, here is a tentative solve. Stress test passes, but error difficult to reproduce as it depends on the overall load on the nodes.

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/rnafusion 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>).
  • 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
Copy link

github-actions bot commented Sep 25, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 55e6444

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

❗ Test warnings:

  • files_exist - File not found: conf/igenomes.config
  • pipeline_todos - TODO string in WorkflowRnafusion.groovy: Optionally add in-text citation tools to this list.

✅ Tests passed:

Run details

  • nf-core/tools version 2.10
  • Run at 2023-10-20 15:07:40

@rannick rannick self-assigned this Sep 25, 2023
@rannick rannick marked this pull request as ready for review September 29, 2023 12:27
@rannick rannick added the WIP Work in progress label Oct 2, 2023
@rannick
Copy link
Collaborator Author

rannick commented Oct 2, 2023

This will need additional test beyond stubs

@rannick rannick added waiting-for-review and removed WIP Work in progress labels Oct 23, 2023
fevac
fevac approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@fevac fevac left a comment

Choose a reason for hiding this comment

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

Nice! 🌟 looks good!

The only thing I was wondering is if we would also want to replace the other picard rules to import from gatk4 instead of picard to not mix both of them. Like for the functions PICARD_COLLECTRNASEQMETRIC and intesertsize. Regardless, if you decide to do that it would be a future pr so go ahead with this 🚀

@rannick rannick merged commit 9fa6eb7 into dev Oct 23, 2023
@rannick
Copy link
Collaborator Author

rannick commented Oct 23, 2023

Good point. However CollectRnaseqMetrics and CollectInsertSize are not duplicated as both picard and gatk modules, only in picard for now. I would observe how it develops in the modules directory in the next months

@rannick rannick deleted the markduplicates_gatk4 branch October 23, 2023 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fevac fevac fevac approved these changes

Assignees

@rannick rannick

Labels
waiting-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rannick @fevac