DSL2: Add mapAD #1072
Open
DSL2: Add mapAD #1072
+446
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
jch-13
force-pushed
the
dsl2-mapad
branch
6 times, most recently
from
8c34dae to
bedff75
Compare
jch-13
force-pushed
the
dsl2-mapad
branch
13 times, most recently
from
a7d8958 to
fd56e8b
Compare
TCLamnidis
requested changes
Sep 27, 2024
There was a problem hiding this comment.
Very good! Just some small changes! 😄
- Rename parameters to match formatting of other params. (they should follow the schema
section_tool_paramatername. i.e.mapping_mapad_gap_read_end_distanceshould bemapping_mapad_gapreadenddistance) - Could you give
mapping_mapad_Da long-form name? I worry that_Dand_dwill be confused by users. - You don't need to run
SAMTOOLS_SORT_MAPADat all. We do a samtools sort after merging all lanes/shards together anyway.
|
Thank you both for the reviews! |
TCLamnidis
approved these changes
Oct 4, 2024
There was a problem hiding this comment.
Great work! Just one small edit to the parameter helptext.
I see you have another PR test left unchecked (ssDNA library). Is that still pending?
Re point 3 of my last review:
I see no SORT calls in the subworkflows of other mappers, but it is entirely possible that they output already sorted bams. I'm happy to take your call on this, was just checking it was intentional and not a forgotten module addition used for testing 👍
|
Oh, good catch. I've verified that. |
jfy133
approved these changes
Oct 11, 2024
nextflow_schema.json
Outdated
| "mapping_mapad_stacklimitabort": { | ||
| "type": "boolean", | ||
| "description": "Set the --stack_limit_abort flag, which instructs mapAD to not try and recover from a full backtracking stack.", | ||
| "help_text": "Without this flag, mapAD tries to recover from a full backtracking stack by removing low-scoring sub-alignments. This can help to recover from difficult to map reads, at the cost of a slower alignment. Setting this flag will speed up the alignment process.\n\n> Modifies mapAD map parameter: `--stack_limit_abort`", |
There was a problem hiding this comment.
I don't really follow what exactly the behaviour does here, and I think the 'backtracking stack' jargon might not be help either.
Suggested change
| "help_text": "Without this flag, mapAD tries to recover from a full backtracking stack by removing low-scoring sub-alignments. This can help to recover from difficult to map reads, at the cost of a slower alignment. Setting this flag will speed up the alignment process.\n\n> Modifies mapAD map parameter: `--stack_limit_abort`", | |
| "help_text": "By default, mapAD will remove low-scoring sub-alignments, helping to recover from difficult to map reads, at the cost of a slower alignment. Setting this flag will speed up the alignment process by not removing these.\n\n> Modifies mapAD map parameter: `--stack_limit_abort`", |
Is that what you mean?
There was a problem hiding this comment.
Right! I'm going to flip the default for this in mapAD and reword this accordingly..
| ch_input_for_mapping.index, | ||
| params.mapping_mapad_p, | ||
| ch_input_for_mapping.strandedness, | ||
| params.mapping_mapad_f, |
There was a problem hiding this comment.
Just to check, all of these parameters would make sense to apply across all samples/genomes in a run?
There was a problem hiding this comment.
Hmmm, right. It would probably make way more sense to make this configurable per sample/genome
|
@jch-13 What is the state of this PR? |
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 3.1.2. For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation. |
|
I'll rebase this after #1106 has landed |
jch-13
force-pushed
the
dsl2-mapad
branch
2 times, most recently
from
d7d70cf to
713a341
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR checklist
scrape_software_versions.pynf-core lint .).nextflow run . -profile test,docker).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).PR TODO
version.ymlgeneration