Sfitz add mosdepth quantize #88
Conversation
…M into merge-test
| mosdepth_quantize_use_fast_algorithm: | ||
| type: 'Bool' | ||
| required: false | ||
| default: false | ||
| help: 'Use fast algorithm for quantizing coverage values' |
There was a problem hiding this comment.
question (non-blocking): Is there a downside to enabling the use of the fast algorithm by default?
There was a problem hiding this comment.
Yeah, this is a difficult decision. Not using fast mode gives more "correct" results. Fast mode ignores paired read overlaps and CIGAR strings (thus indels wrt ref). Ignoring the paired read overlap is the bigger issue, especially for samples with small insert sizes (wrt read length). This is noted in the README. The time difference isn't clear as I haven't benchmarked more than a few samples and not directly on scratch. It's enough that the mosdepth author recommends fast mode for most use-cases (I assume non-small insert cases). We currently have fast mode true by default for the regular mosdepth coverage calculation, so we should probably change one or the other to make them consistent.
test/config/mosdepth-coverage.config
Outdated
| process { | ||
| withName: run_validate_PipeVal { | ||
| when = false | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: This can be removed, the test file should be small enough that the validation shouldn't pose a problem
main.nf
Outdated
| include { assess_coverage_mosdepth } from './module/windows_mosdepth' addParams( | ||
| workflow_output_dir: "${params.output_dir_base}/mosdepth-${params.mosdepth_version}", | ||
| workflow_log_output_dir: "${params.log_output_dir}/process-log/mosdepth-${params.mosdepth_version}" | ||
| ) |
There was a problem hiding this comment.
suggestion: This import is duplicated below; one of them can be removed
module/quantize_mosdepth.nf
Outdated
| export MOSDEPTH_Q0=${params.mosdepth_q0_label} | ||
| export MOSDEPTH_Q1=${params.mosdepth_q1_label} | ||
| export MOSDEPTH_Q2=${params.mosdepth_q2_label} | ||
| export MOSDEPTH_Q3=${params.mosdepth_q3_label} |
There was a problem hiding this comment.
suggestion: Quoting since the parameters are used directly and to make it a bit safer
| export MOSDEPTH_Q0=${params.mosdepth_q0_label} | |
| export MOSDEPTH_Q1=${params.mosdepth_q1_label} | |
| export MOSDEPTH_Q2=${params.mosdepth_q2_label} | |
| export MOSDEPTH_Q3=${params.mosdepth_q3_label} | |
| export MOSDEPTH_Q0="${params.mosdepth_q0_label}" | |
| export MOSDEPTH_Q1="${params.mosdepth_q1_label}" | |
| export MOSDEPTH_Q2="${params.mosdepth_q2_label}" | |
| export MOSDEPTH_Q3="${params.mosdepth_q3_label}" |
module/quantize_mosdepth.nf
Outdated
| path ".command.*" | ||
|
|
||
| script: | ||
| output_filename = generate_standard_filename("mosdepth${params.picard_version}", |
There was a problem hiding this comment.
| output_filename = generate_standard_filename("mosdepth${params.picard_version}", | |
| output_filename = generate_standard_filename("mosdepth-${params.picard_version}", |
|
Test path within initial description has been updated for new test results. |
Description
Testing Results
/hot/software/pipeline/pipeline-generate-SQC-BAM/Nextflow/development/unreleased/sfitz-add-mosdepth-quantize/log-nftest-20241101T231717Z.logChecklist
I have read the code review guidelines and the code review best practice on GitHub check-list.
I have reviewed the Nextflow pipeline standards.
The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)]-[brief_description_of_branch].
I have set up or verified the branch protection rule following the github standards before opening this pull request.
I have added my name to the contributors listings in the
manifestblock in thenextflow.configas part of this pull request, am listedalready, or do not wish to be listed. (This acknowledgement is optional.)
I have added the changes included in this pull request to the
CHANGELOG.mdunder the next release version or unreleased, and updated the date.I have updated the version number in the
metadata.yamlandmanifestblock of thenextflow.configfile following semver, or the version number has already been updated. (Leave it unchecked if you are unsure about new version number and discuss it with the infrastructure team in this PR.)I have tested the pipeline using NFTest, or I have justified why I did not need to run NFTest above.