feat: add complement events #382
Conversation
…`artifact` probability
…level, as it is parsed
…eport activation logic and automatically annotate what's needed, only when report is activated)
WalkthroughThis update restructures and expands the configuration and workflow logic for FDR control and event handling in variant calling. It introduces complement events, refines artifact retention parameters, generalizes event wildcards to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Snakemake
participant Config
participant FilteringRules
participant DatavzrdRules
User->>Snakemake: Trigger workflow
Snakemake->>Config: Load config.yaml (with events & complement_events)
Snakemake->>FilteringRules: Run merge_exclude_events (if complement event defined)
FilteringRules->>FilteringRules: Merge BCFs for excluded events
Snakemake->>FilteringRules: Run complement_event (set subtraction)
FilteringRules->>FilteringRules: bcftools isec (full event - exclude set)
Snakemake->>DatavzrdRules: Process variant/fusion calls with {any_event}
DatavzrdRules->>Config: Resolve event via get_any_event
DatavzrdRules->>DatavzrdRules: Generate reports and outputs for each event/complement event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Also, the branching off of an older release version is intended, as we need this functionality in a workflow, where we don't want to re-trigger analyses that later changes in the workflow would trigger. But we can update this branch, once we have reviewed everything and before we merge it. But then the user can point to the last commit that works for them. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
workflow/rules/common.smk (1)
1512-1512: Consider following PEP 8 spacing conventions.The removal of spaces around the subtraction operator goes against PEP 8 style guidelines which recommend spaces around binary operators.
- extra += f" -T {min_primer_len-2}" + extra += f" -T {min_primer_len - 2}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.test/config-sra/config.yaml(3 hunks)config/config.yaml(1 hunks)workflow/resources/datavzrd/fusion-calls-template.datavzrd.yaml(1 hunks)workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml(1 hunks)workflow/rules/common.smk(8 hunks)workflow/rules/datavzrd.smk(8 hunks)workflow/rules/filtering.smk(1 hunks)workflow/rules/maf.smk(1 hunks)workflow/rules/table.smk(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.test/config-sra/config.yaml
[error] 58-58: trailing spaces
(trailing-spaces)
[error] 61-61: trailing spaces
(trailing-spaces)
[error] 69-69: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test no candidate filtering
- GitHub Check: test sra download
- GitHub Check: test local input
- GitHub Check: test target regions, multiple BEDs
- GitHub Check: test primers
- GitHub Check: test testcase generation
- GitHub Check: test target regions
🔇 Additional comments (18)
workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml (1)
3-3: LGTM! Wildcard update aligns with complement events feature.The change from
wildcards.eventtowildcards.any_eventcorrectly supports the new event handling that includes both standard and complement events.workflow/resources/datavzrd/fusion-calls-template.datavzrd.yaml (1)
3-3: LGTM! Consistent wildcard update.The wildcard change matches the pattern in
variant-calls-template.datavzrd.yamland properly supports the unified event handling.config/config.yaml (1)
171-174: LGTM! Improved configuration granularity for artifact retention.Moving
retain-artifactsfrom global to event-specific scope provides the flexibility needed for complement events functionality, where different events may need different artifact retention behaviors.workflow/rules/maf.smk (2)
36-36: LGTM! Minor formatting improvement.Removing extra spaces around parameter assignments improves code consistency.
41-41: LGTM! Consistent formatting cleanup.Matches the formatting improvement applied to the previous
lookupcall.workflow/rules/table.smk (3)
3-3: LGTM! Input path updated for unified event handling.The wildcard change supports both standard and complement events in the table generation workflow.
6-6: LGTM! Output path consistent with wildcard changes.Properly updated to use the
any_eventwildcard for unified event handling.
12-12:✅ Verification successful
Verify that
any_eventwildcard is properly defined.The log path update is consistent with other changes. However, ensure that the
any_eventwildcard is properly defined in the workflow rules to handle both standard and complement events.
🏁 Script executed:
#!/bin/bash # Description: Verify that any_event wildcard is defined and used consistently across the workflow # Check for any_event wildcard definition and usage echo "=== Searching for any_event wildcard definition ===" rg -A 5 -B 5 "any_event.*=" --type py echo -e "\n=== Searching for any_event wildcard usage in Snakemake rules ===" rg "any_event" --type-add 'smk:*.smk' --type smk echo -e "\n=== Checking for any remaining old event wildcards ===" rg "\{event\}" --type-add 'smk:*.smk' --type smkLength of output: 9418
✅ Confirmed
any_eventwildcard definition and usage
- The
any_eventwildcard is defined inworkflow/rules/common.smk(viaany_event="|".join(...)) and handled inget_any_eventfor both standard and complement events.workflow/rules/table.smk’s log path (logs/vembrane-table/{group}.{any_event}.{calling_type}.log) matches the established pattern and is used consistently across other rules.No further action required.
workflow/rules/filtering.smk (2)
98-111: LGTM! Well-structured rule for merging exclude events.The implementation correctly uses dynamic input generation and appropriate wrapper version for bcftools concat operation.
146-156: Wildcard update correctly implemented.The systematic replacement of
{event}with{any_event}properly extends the rule to handle both regular and complement events..test/config-sra/config.yaml (2)
85-93: Well-designed complement events configuration.The
filtered_artifactsevent is properly configured to capture variants removed during FDR control, which aligns perfectly with the PR objectives of generating reports for filtered-out candidates.
107-109: Verify the impact of broadening VEP annotations.Changing from
--fields IMPACTto--everythingsignificantly increases the annotation scope, which may impact performance and output size.Consider whether all annotations are necessary for your use case, as this change could substantially increase processing time and storage requirements.
workflow/rules/datavzrd.smk (2)
10-10: Improved error handling with lookup function.Good use of the
lookupfunction with a default empty list, making the code more robust when sort configuration is missing.
100-100: Consistent error handling for event descriptions.The lambda function with
lookupand default description ensures the report generation doesn't fail when event descriptions are missing.workflow/rules/common.smk (4)
144-153: Proper integration of complement events with backward compatibility.The function correctly extends event collection to include complement events while maintaining compatibility with configurations that don't define them.
460-472: Well-implemented input generation function.The function follows established patterns and correctly generates input file paths for events to be excluded from complement events.
1002-1014: Correct wildcard constraints for event handling.The use of symmetric difference (
^) forany_eventproperly ensures mutual exclusivity between regular and complement events.
1559-1564: Clean abstraction for event configuration retrieval.The
get_any_eventfunction provides a simple and effective way to retrieve event configuration regardless of whether it's a regular or complement event.
|
Can't you get the same by defining an ordinary callset/filter-event that selects everything with an artifact probability of least 5%? |
|
I think not. Consider a variant with these (decoded) probabilities: With an FDR of If we set I know this is a deliberate corner case, but its existence will mean that we will sometimes miss filtered artifacts if we just do a filtering based on the Also, there might be other scenarios, where we might want to do complementing of sets, because we might have a scenario with lots of fine-grained probabilities and we want to figure out which variants will be excluded if we just exclude one of those fine-grained probabilities from the call-set definition. |
|
I see the point. Maybe it would be more convenient and less complex to maintain as a functionality of the varlociraptor control-fdr subcommand though? In the sense of that you can specify additional output files there for storing stuff that was filtered out, stratified by the strongest non-target event? |
|
Yes, that is a good idea. Basically have the option for varlociraptor to also output everything that it removes during fdr-control, and output one file per non-target varlociraptor (scenario) event, including Then I guess we will not merge this pull request here, but I would like to keep the branch around for our current analysis. |
The use case that motivates this, is to generate a datavzrd report table for all the candidate variants that were filtered out due to their
PROB_ARTIFACTduring FDR control filtering.The only way that I see to cleanly do this, is to generate two variant sets:
retain-artifacts: trueset.We can then take the complement of 1 in 2, to extract those candidate variants that are only found in 2 (with
retain-artifacts: true).This functionality might also be interesting for other debugging scenarios or intensive inspection of variant sets, but is probably not common enough to warrant putting it in the main default
config/config.yamlthat comes with everysnakedeployment as a module.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Style