Add optional merging and trimming

[maxibor]

In response to #137

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

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

[apeltzer]

Atm, this doesn't allow skipping the entire module and thus conflicts a bit with the open PR #138 - could you adjust these to also allow an entire skipping of the AR2 process in general, not just the merging part? Some people might want to run adapter clipping but without merging (which is now possible), others might just want to skip AR entirely - which isn't possible yet (see other PR #138). I guess it makes more sense to include #138 into this PR here and make it work 👍

Thanks for contributing, much appreciated!

[maxibor]

Integrating PR #138
Now inivudualized to:

• skip_collapse
• skip_trim

It it now possible to:

• trim but not collapse
• collapse but not trim
• avoid collapssing and trimming alltogether (skips ar2)

I went with the "lazy" option of skipping ar2 by changing the script of the ar2 process to just a simple mv.
However, it is suboptimal and might need some rewriting later with something like that.

Also, I think there is an issue with the test of damageprofiler that always errors with the last test (from the bam file)
nextflow run ${TRAVIS_BUILD_DIR} -profile testbam,docker --bam

[jfy133]

I think it would be clearer/more consistent to have a --skip_adapterremoval flag here (which switches both --skip_collapse and --skip_trim to false).

Then we move the --skip_collapse and --skip_trim in help message to 'AdapterRemoval' section, as in that section you're actually modifying the module, not turning entire modules on and off.

[apeltzer]

Hi @maxibor @jfy133 ! I'll go through the code asap and will adjust the testcase or figure out why it fails now. Probably something smaller, but anyways a good point to check it first 👍

[apeltzer]

Sorry for not looking at this earlier - I'll take some time very soon to get this in and comment on the various points to get this going 👍

[apeltzer]

I fixed the merge conflicts and we'll see if things work fine now - once this passes I think I'm already pretty happy about it - maybe some option renaming as @jfy133 suggested and some docs / changelog updates and we're almost good to go 👍

[apeltzer]

Ok, bam input is broken now - everything else works fine - @maxibor do you want to take a look at this and maybe also have the command line switches as suggested by @jfy133 implemented?

[maxibor]

Ok, bam input is broken now - everything else works fine - @maxibor do you want to take a look at this and maybe also have the command line switches as suggested by @jfy133 implemented?

• Matched @jfy133 skip_* pattern suggestion
• Updated test
• Updated doc
• Local executor selection when skip_adapterremoval is set to true to avoid waiting for job scheduler (SLURM, SGE, ...) to allocate ressources for a simple mv

[jfy133]

Looks good me now. Have you tested it locally yourself? I've never had any cases where I needed to skip collapse or merge so I cant see what happens downstream. Edit: meant collapse or trim

I'll try and test the skip module itself this evening. Edit2: Confirm --skip_adapterremoval works with no issue on my test case.

[apeltzer]

I guess we could actually use a mix operator here to get around the local executor. Might have a look too :-)

[jfy133]

Note to self once merged: add to docs saying that if --skip_adapterremoval selected by user, the input FASTQs must be with --singleEnd

[apeltzer]

Is that true? I mean it wouldn't be a big issue to make both bwa, bwa alncapable of running in PE mode then for these cases as we have the command line switch already there and present. I could even add specific tests to test this behaviour as well... ?

[jfy133]

Is that true? I mean it wouldn't be a big issue to make both bwa, bwa alncapable of running in PE mode then for these cases as we have the command line switch already there and present. I could even add specific tests to test this behaviour as well... ?

Partly true. That note would admittedly be aimed more more novice users (I will switch out 'must' with 'generally will be'). But I personally can't think of any case where you would run paired end data without merging/trimming. Although on second thoughts that's maybe what Alex Huebner was partly referring to with the whole modern data issue in #64 ...

At least in my experience the only time when I wanted to skip AR was when I had already processed it previously and wanted to map the merged/trimmed data to a different reference genome.

[apeltzer]

Yes, but as we already have everything configurable now - I'm working on adding the purely modern handling (just apply trimming to reads, keep them as pairs and map them using bwa/bwa mem in PE mode) as an option in the pipeline too. This will just require speciying --skip_collapse when having PE data, then just trimming will be applied and mapping will happen in PE mode as one would do for modern data 👍

[apeltzer]

maxibor#1

Here you go - once this is merged, we can basically just add something on top of it :-)

[apeltzer]

I merged this into the ar_possible branch and will merge my changes on top of this - this should make it possible for both of us to have a final check before merging to dev in the end!