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

Speed up strip_input_fastq process #327

Merged
merged 9 commits into from Jan 9, 2020
Merged

Conversation

maxibor
Copy link
Member

@maxibor maxibor commented Jan 7, 2020

Rewrite of extract_map_reads.py to speed it up and make it more robust:

  • Fastq parsing now handled by Biopython
  • Fastq I/O now handled with xopen
  • Now handles better forward and reverse reads files.
  • Lowers resources requirements for strip_input_fastq

Speed improvement

Wall time before (strip_input_fastq in green):

strip_before

Wall time after (strip_input_fastq in lightblue):

strip_after

PR checklist

  • This comment contains a description of changes (with reason)
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Make sure your code lints (nf-core lint .).
  • CHANGELOG.md is updated

Learn more about contributing: https://github.com/nf-core/eager/tree/master/.github/CONTRIBUTING.md

@maxibor maxibor requested a review from jfy133 January 7, 2020 14:53
@maxibor maxibor changed the title Speed up strip fastq Speed up strip_input_fastq Jan 7, 2020
@maxibor maxibor changed the title Speed up strip_input_fastq Speed up strip_input_fastq process Jan 7, 2020
@jfy133
Copy link
Member

jfy133 commented Jan 8, 2020

I don't see any issue code wise. I will run a big test on sdag to double check then will merge.

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.

Looks good, mybig test shows massive improvement.

One small request: please move the publishDir outside samtools/. An independent directory is valid IMO.

Then we are good for merging!

@maxibor
Copy link
Member Author

maxibor commented Jan 9, 2020

Looks good, mybig test shows massive improvement.

One small request: please move the publishDir outside samtools/. An independent directory is valid IMO.

Then we are good for merging!

Done

@maxibor maxibor requested a review from jfy133 January 9, 2020 13:33
@jfy133
Copy link
Member

jfy133 commented Jan 9, 2020

Merging - strip_fastq passes, tests hung on GATK UG Download which happens sometimes.

@jfy133 jfy133 merged commit 1495485 into nf-core:dev Jan 9, 2020
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.

None yet

2 participants