feat: enable event reports #364
Conversation
WalkthroughThe pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant G as get_final_output
participant L as lookup
G->>L: Call lookup(path, default=True)
alt Path Found
L-->>G: Return actual value
else Path Not Found
L-->>G: Return True
end
G->>G: Process result into final_output
Possibly related PRs
Suggested reviewers
Poem
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: 0
🧹 Nitpick comments (1)
workflow/rules/common.smk (1)
166-207: Consider refactoring the nested conditional logic.The report generation logic contains deeply nested conditionals that make it harder to understand the control flow. Consider extracting the report generation logic into a separate function for better readability and maintainability.
- for calling_type in calling_types: - if config["report"]["activate"]: - for event in get_calling_events(calling_type): - if lookup( - dpath=f"calling/fdr-control/events/{event}/report", - within=config, - default=True, - ): - final_output.extend( - expand( - "results/datavzrd-report/{batch}.{event}.{calling_type}.fdr-controlled", - batch=get_report_batches(calling_type), - event=event, - calling_type=calling_type, - ) - ) - else: - final_output.extend( - expand( - "results/final-calls/{group}.{event}.{calling_type}.fdr-controlled.bcf", - group=( - variants_groups - if calling_type == "variants" - else fusions_groups - ), - event=event, - calling_type=calling_type, - ) - ) +def get_report_outputs(calling_type, event): + """Generate report outputs for a given calling type and event.""" + if not config["report"]["activate"]: + return [] + + if lookup( + dpath=f"calling/fdr-control/events/{event}/report", + within=config, + default=True, + ): + return expand( + "results/datavzrd-report/{batch}.{event}.{calling_type}.fdr-controlled", + batch=get_report_batches(calling_type), + event=event, + calling_type=calling_type, + ) + else: + return expand( + "results/final-calls/{group}.{event}.{calling_type}.fdr-controlled.bcf", + group=( + variants_groups + if calling_type == "variants" + else fusions_groups + ), + event=event, + calling_type=calling_type, + ) +def get_final_output(wildcards): + final_output = expand( + "results/qc/multiqc/{group}.html", + group=groups, + ) + + final_output.extend( + expand( + "results/datavzrd-report/{group}.coverage", + group=groups, + ) + ) + + for calling_type in calling_types: + for event in get_calling_events(calling_type): + final_output.extend(get_report_outputs(calling_type, event))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workflow/rules/common.smk(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Formatting
🔇 Additional comments (2)
workflow/rules/common.smk (2)
172-172: Approve the change to enable reports by default.This change aligns with the PR objectives by making report generation opt-out instead of opt-in when reports are globally enabled. This fixes the issue where reports were not being generated even when activated in the configuration.
167-181:❓ Verification inconclusive
Verify the impact of enabling reports by default.
The change affects the report generation logic when
config["report"]["activate"]isTrue. Now, if an event's report configuration is not explicitly set toFalse, a report will be generated. This could lead to unexpected report generation for events that previously didn't have reports.Let's verify the potential impact:
🏁 Script executed:
#!/bin/bash # Description: Check for events that might be affected by the default value change # Search for event configurations that don't explicitly set the report property ast-grep --pattern $'calling: fdr-control: events: $_: $$$' # Search for any existing report configurations rg -A 5 'report:'Length of output: 276
Subject: Verify Default Report Generation Behavior in
common.smkThe code now uses a lookup with a default value of True, which means that if an event’s configuration does not explicitly set
reporttoFalse, a report will be generated. Our repository search (via the provided shell scripts) did not reveal any explicit disabling of reports for events. This indicates that enabling reports by default may lead to a broader set of reports than previously expected.
- Impact: Events lacking an explicit report configuration will now generate reports, which could be unintentional.
- Action: Double-check the event configuration files and related settings (e.g., in
workflow/Snakefile) to ensure this default behavior aligns with your intended workflow.
Currently the workflow does not create any reports when reports are enabled in the config.
This happens as we have a global option for creating reports by checking
config["report"]["activate"].Still, when enabling this option no report will be created as for every single event the workflow checks if it contains a report property and defaults to
Falseif non exists.This is misleading as the global option indicates that reports will be created while it is currently opt-in instead of opt-out for every single event.
Summary by CodeRabbit