fix: processing vembrane config #330
Conversation
WalkthroughThe changes in this pull request involve updates to multiple Snakemake workflow files, focusing on version upgrades for wrappers, enhancements in sample data handling, and the introduction of new rules. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Workflow
participant Freebayes
participant Delly
participant Bcftools
User->>Workflow: Initiate variant calling
Workflow->>Freebayes: Execute with updated parameters
Freebayes-->>Workflow: Return variant calls
Workflow->>Delly: Execute variant calling
Delly-->>Workflow: Return variant calls
Workflow->>Bcftools: Filter invalid BCFs
Bcftools-->>Workflow: Return valid BCFs
Workflow-->>User: Provide final variant calls
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
workflow/rules/plugins.smk (1)
9-9: Approve the URL change with suggestions for improvementThe change to use Zenodo as the download source for REVEL scores is a good improvement in terms of reliability and accessibility. This aligns well with the PR objective of addressing the unreliable download link issue.
However, I have a couple of suggestions to further enhance this change:
- Consider parameterizing the URL or at least the version number. This would make future updates easier to manage. For example:
params: revel_version = "v1.3", zenodo_record = "7072866" shell: "curl https://zenodo.org/records/{params.zenodo_record}/files/revel-{params.revel_version}_all_chromosomes.zip -o {output} &> {log}"
- It would be beneficial to add a checksum verification step to ensure the integrity of the downloaded file. This could be implemented using a separate rule or by adding a verification step within this rule.
Would you like me to provide an example of how to implement the checksum verification?
workflow/rules/candidate_calling.smk (3)
Line range hint
39-47: New rule addresses the Delly breakends issue effectively.The
fix_delly_callsrule successfully filters out breakends from Delly output, addressing the issue of invalid BCFs after VEP annotation. This aligns well with the PR objectives.Consider adding error handling to check if the input file exists and is not empty before processing:
rule fix_delly_calls: input: "results/candidate-calls/{group}.delly.bcf", output: "results/candidate-calls/{group}.delly.no_bnds.bcf", log: "logs/fix_delly_calls/{group}.log", conda: "../envs/bcftools.yaml" shell: - """bcftools view -e 'INFO/SVTYPE="BND"' {input} -Ob > {output} 2> {log}""" + """ + if [ ! -s {input} ]; then + echo "Error: Input file {input} is empty or does not exist" >&2 + exit 1 + fi + bcftools view -e 'INFO/SVTYPE="BND"' {input} -Ob > {output} 2> {log} + """
Line range hint
50-62: Improved input handling for off-target variant filtering.The
filter_offtarget_variantsrule now uses theget_fixed_candidate_callsfunction, which likely includes the output from the newfix_delly_callsrule. This ensures that only valid calls (with breakends removed) are processed, aligning with the PR objectives.For improved clarity, consider adding a comment explaining the
get_fixed_candidate_callsfunction:rule filter_offtarget_variants: input: + # get_fixed_candidate_calls returns the output of fix_delly_calls for Delly, + # and the original calls for other callers calls=get_fixed_candidate_calls("bcf"), index=get_fixed_candidate_calls("bcf.csi"), regions="resources/target_regions/target_regions.bed", output: "results/candidate-calls/{group}.{caller}.filtered.bcf", params: extra="", log: "logs/filter_offtarget_variants/{group}.{caller}.log", wrapper: "v2.3.2/bio/bcftools/filter"
Line range hint
65-79: Improved flexibility in candidate call scattering.The
scatter_candidatesrule now conditionally uses either filtered calls (if target regions are defined) or fixed calls. This change allows for more flexibility in the workflow, accommodating both targeted and whole-genome analyses, which improves the overall robustness of the workflow.To improve readability, consider using a ternary operator for the input definition:
rule scatter_candidates: input: - "results/candidate-calls/{group}.{caller}.filtered.bcf" - if config.get("target_regions", None) - else get_fixed_candidate_calls("bcf"), + lambda wildcards: ( + "results/candidate-calls/{group}.{caller}.filtered.bcf" + if config.get("target_regions", None) + else get_fixed_candidate_calls("bcf") + ).format(**wildcards), output: scatter.calling( "results/candidate-calls/{{group}}.{{caller}}.{scatteritem}.bcf" ), log: "logs/scatter-candidates/{group}.{caller}.log", conda: "../envs/rbt.yaml" shell: "rbt vcf-split {input} {output}"workflow/rules/common.smk (1)
Line range hint
464-465: Potential bug:get_readsfunction ignoresfqparameterIn the
get_cutadapt_inputfunction, theget_readsfunction defined inside theelseclause ofif len(fq1) == 1uses a hardcoded filename ending with.fq1.fastq, regardless of thefqargument passed to it. This means that whenget_readsis called with'fq2', it will still return a path ending with.fq1.fastq, which may not be correct for paired-end data.Apply this diff to fix the issue:
-def get_reads(fq): - return f"pipe/cutadapt/{unit.sample_name}/{unit.unit_name}.fq1.fastq{ending}" +def get_reads(fq): + return f"pipe/cutadapt/{unit.sample_name}/{unit.unit_name}.{fq}.fastq{ending}"This change ensures that the
fqparameter is correctly used in constructing the file path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- workflow/rules/candidate_calling.smk (1 hunks)
- workflow/rules/common.smk (1 hunks)
- workflow/rules/mapping.smk (1 hunks)
- workflow/rules/plugins.smk (1 hunks)
🔇 Additional comments (4)
workflow/rules/plugins.smk (1)
9-9: Verify consistency of the downloaded fileWhile the change in the download URL is an improvement, it's crucial to ensure that the file downloaded from Zenodo has the exact same format and content as the one from the previous source. This is to guarantee that the subsequent rules (
process_revel_scoresandtabix_revel_scores) continue to function correctly.To verify this, you could run the following script:
This script will help ensure that the new source provides consistent data, maintaining the integrity of the workflow.
workflow/rules/candidate_calling.smk (2)
Line range hint
1-79: Overall, the changes effectively address the PR objectives.The modifications to the
workflow/rules/candidate_calling.smkfile successfully improve the robustness and flexibility of the workflow. The newfix_delly_callsrule addresses the issue with Delly breakends, while the updates tofilter_offtarget_variantsandscatter_candidatesrules enhance the workflow's ability to handle different scenarios. The freebayes wrapper update is a good practice, but requires verification of compatibility.These changes align well with the PR objectives of enhancing the workflow's robustness and preventing failures related to custom annotation fields. The suggestions provided are minor and aimed at improving clarity and error handling.
23-23: Verify compatibility with the updated freebayes wrapper.The update from v2.7.0 to v4.4.0 is a significant version jump. While this is generally good for leveraging new features and bug fixes, it's important to ensure compatibility with the rest of the workflow.
Please run the following script to check for any breaking changes or new requirements:
workflow/rules/mapping.smk (1)
86-86: Verify the impact of updating the Picard MarkDuplicates wrapper version.The wrapper version for the
mark_duplicatesrule has been updated from v2.5.0 to v4.2.0. This is a significant version bump that may introduce new features, bug fixes, or changes in behavior.Please ensure that:
- This change is intentional and aligns with the PR objectives.
- The new version is compatible with the rest of the workflow.
- No additional changes are required in the rule or elsewhere in the workflow due to this update.
It's recommended to:
- Review the changelog or release notes for the new version to understand the changes.
- Test the workflow thoroughly with this new version to ensure compatibility and expected behavior.
To help verify the changes, you can run the following script:
This script will help identify any documentation about the version change, ensure consistent usage of the new version, and check for any related TODO or FIXME comments.
There was a problem hiding this comment.
This looks good to me. If you have checked, that this also ends up putting the exon correctly into the report for your use case, I guess this is good to go.
The only thing you might want to double-check, is whether the wrapper updates are really necessary. These would trigger reruns in existing workflows updated to the resulting new workflow version, so I would only do them if necessary for this PR.
workflow/rules/candidate_calling.smk
Outdated
| @@ -20,7 +20,7 @@ rule freebayes: | |||
| ), | |||
| threads: max(workflow.cores - 1, 1) # use all available cores -1 (because of the pipe) for calling | |||
| wrapper: | |||
| "v2.7.0/bio/freebayes" | |||
| "v4.4.0/bio/freebayes" | |||
There was a problem hiding this comment.
Is this necessary for anything (like a bug fix in a newer version of freebayes or the wrapper)? Because otherwise I would leave this as is, so as not to trigger rerunning of the candidate calling in workflows that update to a newer release after merging this PR...
| info_prob_fields, | ||
| rename_info_fields, | ||
| lambda x: f"INFO['PROB_{x.upper()}']", | ||
| "prob: {}".format, |
workflow/rules/mapping.smk
Outdated
| @@ -83,7 +83,7 @@ rule mark_duplicates: | |||
| #https://broadinstitute.github.io/picard/faq.html | |||
| mem_mb=3000, | |||
| wrapper: | |||
| "v2.5.0/bio/picard/markduplicates" | |||
| "v4.2.0/bio/picard/markduplicates" | |||
There was a problem hiding this comment.
As above: if such an update is not really necessary for anything in this pull request, I would defer updating to whenever this is really necessary. Otherwise this will trigger reruns in previously workflows where we update to a newer workflow version...
| @@ -6,7 +6,7 @@ rule download_revel: | |||
| conda: | |||
| "../envs/curl.yaml" | |||
| shell: | |||
| "curl https://rothsj06.dmz.hpc.mssm.edu/revel-v1.3_all_chromosomes.zip -o {output} &> {log}" | |||
| "curl https://zenodo.org/records/7072866/files/revel-v1.3_all_chromosomes.zip -o {output} &> {log}" | |||
There was a problem hiding this comment.
Nice, let's hope this stays more robust in the long-term and that newer versions will also end up on Zenodo!
Just some smaller fixes.
The link for downloading revel scores is broken from time to time. The scores are also hosted on zenodo which should be more reliable.
In addition I also changed parsing the exon entry as
ANN['exon].rawas the default output would be e.gnumber / total: 2 / 5. Calling it with.rawattribute will output2/5which is much more readable in a datavzrd report.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
freebayes,delly, andPicard, improving functionality and compatibility.Bug Fixes