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

Flexible AdapterRemoval #159

Merged
merged 39 commits into from
Mar 5, 2019
Merged

Flexible AdapterRemoval #159

merged 39 commits into from
Mar 5, 2019

Conversation

apeltzer
Copy link
Member

@apeltzer apeltzer commented Mar 4, 2019

This is based on @maxibor and @jfy133 s work and help and should provide the means to run:

  • AdapterRemoval v2 on PE/SE data
  • with or without read merging /collapsing
  • with or without read trimming
  • Skipping the entire module if no need for the module in general.

Thereby addressing #137, #64.

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/eager 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

This might require some docs updating (open a PR against the ar_optional branch here) and some more testing, as I adjusted some stuff in my PR on top of @maxibor 's.

Thanks A BUNCH for helping out on this both!

Copy link
Member

@phue phue left a comment

Choose a reason for hiding this comment

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

Nice work!
This is just a bit of nitpicking, also I noticed the somewhat inconsistent use of adapter vs. adaptor.
I know different tools use different spellings but I think it would be nice to have it consistent for the pipeline params

main.nf Outdated Show resolved Hide resolved
main.nf 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
phue and others added 3 commits March 5, 2019 08:45
Co-Authored-By: apeltzer <[email protected]>
Co-Authored-By: apeltzer <[email protected]>
Co-Authored-By: apeltzer <[email protected]>
@apeltzer
Copy link
Member Author

apeltzer commented Mar 5, 2019

I'll be cleaning up the trimming part - its possible to have a predefined command in there for SE/PE mode and get rid of some boilerplate code this way 👍

@apeltzer
Copy link
Member Author

apeltzer commented Mar 5, 2019

Unfortunately, the output handling with AdapterRemoval makes it a bit complicated to be able to accomodate all cases (PE/SE) into a single condensed version:

  • depending whether you run with --collapse, its creating different fastq files as output
  • depending on whether you use the trimming, the same happens as well

So I can't really combine individual steps in all cases, but rather have the same merging and quality trimming options in externalized commans that I use in all cases (which works fine 👍 ) to make the code a bit cleaner and easier to read.

I guess once the tests pass, we can basically assume this is fine 👍

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