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

Allocate more RAM to the process running MACS2 #48

Merged
merged 5 commits into from
Sep 30, 2019

Conversation

jinmingda
Copy link
Contributor

@jinmingda jinmingda commented Sep 29, 2019

Many thanks to contributing to nf-core/atacseq!

Please fill in the appropriate checklist below (delete whatever is not relevant). These are the most common things requested on pull requests (PRs).

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 necessary, also make a PR on the nf-core/atacseq 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: https://github.com/nf-core/atacseq/tree/master/.github/CONTRIBUTING.md

Description

The memory footprint of macs2 is largely based on the size of the input BAM file. The label process_long used in "merge_library_macs" and "merge_replicate_macs" can only assign 2GB to the processes, which is hardly enough for many real human samples.

@jinmingda jinmingda changed the title Dev macs Allocate more RAM to the process running MACS2 Sep 29, 2019
Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Perfect 👍 Can you update CHANGELOG too please. Thanks.

@drpatelh
Copy link
Member

Would you mind also adding a separate PR (or you can do it here if you like) increasing the resource requirements for all of the annotatePeaks processes? After we have merged all of the changes it would be good if you are able to test the dev version of the pipeline using the -r dev parameter. Hopefully, it finishes without errors this time!

@drpatelh
Copy link
Member

Added in BED download functionality from AWS iGenomes in the PR below along with some other minor changes:
#49

@drpatelh drpatelh merged commit 44e1d74 into nf-core:dev Sep 30, 2019
@jinmingda
Copy link
Contributor Author

Hi @drpatelh, sorry I couldn't reply back to you asap since I wasn't at home yesterday. I will take a look at your review and work on the code later today.

@drpatelh
Copy link
Member

Hi @jinmingda . No worries! Ive incorporated all the changes you suggested now. Can you check the open PR to see ive missed anything please and I'll add it in 👍 Thanks again.

@jinmingda
Copy link
Contributor Author

@drpatelh I think your PR has incorporated everything I've found. However, it seems the description of using the staged blacklist file in makeGenomeFilter is not included in the CHANGELOG.

@drpatelh
Copy link
Member

Good point. I'll add it in tomorrow. Thanks.

@drpatelh
Copy link
Member

If there are any other improvements that you think that the pipeline would benefit from then let me know and I'll try and add them in. Probably going to do a minor release over the next couple of weeks to get the pipeline running on AWS without issue.

@jinmingda
Copy link
Contributor Author

@drpatelh Sure! I will keep you informed if I find any bugs or things that can improve the pipeline.

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.

3 participants