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

Add optional trimming with Trim Galore #117

Merged
merged 60 commits into from
Feb 26, 2020
Merged

Add optional trimming with Trim Galore #117

merged 60 commits into from
Feb 26, 2020

Conversation

chelauk
Copy link
Contributor

@chelauk chelauk commented Feb 14, 2020

nf-core/sarek pull request

Many thanks for contributing to nf-core/sarek!

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/sarek 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: CONTRIBUTING.md

@maxulysse maxulysse changed the title Dev Add optionnal trimming with Trim Galore Feb 17, 2020
@maxulysse maxulysse self-assigned this Feb 17, 2020
@maxulysse
Copy link
Member

Hi @chelauk that looks like a very good start.

Could you add params to specify trimming options?

I think the rnaseq pipeline could be a good place to look at what to use already:
https://github.com/nf-core/rnaseq/blob/7c3aa75ec25db0d12a66181aed4fcbacf4b9901e/main.nf#L49-L57

Instead of --skipTrimming I would allow trimming only if said params are specified.
Do you think you could do that?

@chelauk
Copy link
Contributor Author

chelauk commented Feb 18, 2020 via email

include 3' and 5' clipping options for Trim Galore before and after adapter clipping
include publishDir options to fastqTrim process
minor edit, uncommented --saveTrimmed option
minor edit; corrected nextflow config trimFastq options
@chelauk chelauk changed the title Add optionnal trimming with Trim Galore Add optional trimming with Trim Galore Feb 19, 2020
@chelauk
Copy link
Contributor Author

chelauk commented Feb 19, 2020

Could you add params to specify trimming options?

Yes

I think the rnaseq pipeline could be a good place to look at what to use already:
https://github.com/nf-core/rnaseq/blob/7c3aa75ec25db0d12a66181aed4fcbacf4b9901e/main.nf#L49-L57

Instead of --skipTrimming I would allow trimming only if said params are specified.
Do you think you could do that?

yes added --fastqTrim option

@chelauk chelauk marked this pull request as ready for review February 19, 2020 13:04
@maxulysse
Copy link
Member

I just edited your comments for clarity

main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
chelauk and others added 3 commits February 19, 2020 13:10
Co-Authored-By: Maxime Garcia <maxime.garcia@scilifelab.se>
Co-Authored-By: Maxime Garcia <maxime.garcia@scilifelab.se>
Co-Authored-By: Maxime Garcia <maxime.garcia@scilifelab.se>
@chelauk
Copy link
Contributor Author

chelauk commented Feb 24, 2020

when I use this command line I now get an error
nextflow run sarek -profile test,singularity --trim_fastq --clip_r1 2 --clip_r2 2

I am getting this error:


Caused by:
  Unknown variable 'clip_r1' -- Make sure it is not misspelt and defined somewhere in the script before using it

in nextflow.config it is defined:

params { 

 stuff = 'foo'
// options: Trimming
  trim_fastq = false
  clip_r1 = 0
  clip_r2 = 0
  three_prime_clip_r1 = 0
  three_prime_clip_r2 = 0
  trim_nextseq = 0
  skip_trimming = false
  save_trimmed = false

}

main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
main.nf Outdated Show resolved Hide resolved
Co-Authored-By: Maxime Garcia <maxime.garcia@scilifelab.se>
@chelauk
Copy link
Contributor Author

chelauk commented Feb 24, 2020

nextflow run sarek -profile test,singularity --trim_fastq --clip_r1 2 --clip_r2 2 -with-singularity 'sifs/sarek.sif'
runs ok

@maxulysse maxulysse added this to the 2.6 milestone Feb 25, 2020
main.nf Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
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.

Looks good @chelauk @maxulysse ! Couple of minor comments left. Ill approve so you can merge after fixing 👍

chelauk and others added 2 commits February 25, 2020 12:23
Co-Authored-By: Harshil Patel <drpatelh@users.noreply.github.com>
delete skip_trimming = false
@chelauk
Copy link
Contributor Author

chelauk commented Feb 25, 2020

sorry accidentally requested a review from @apeltzer

Co-Authored-By: Maxime Garcia <maxime.garcia@scilifelab.se>
main.nf Outdated Show resolved Hide resolved
main.nf Show resolved Hide resolved
chelauk and others added 3 commits February 25, 2020 15:51
Co-Authored-By: Maxime Garcia <maxime.garcia@scilifelab.se>
Co-Authored-By: Maxime Garcia <maxime.garcia@scilifelab.se>
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

Looks perfect

@maxulysse maxulysse merged commit 8972a6c into nf-core:dev Feb 26, 2020
@chelauk chelauk deleted the dev branch February 26, 2020 12:56
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

4 participants