-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Update of datavzrd Version for boolean spell and use of clin_sig spell #342
Conversation
… of the clin_sig spell instead of function
WalkthroughThe changes introduced in the pull request primarily affect the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml (1)
480-483
: Consider DRY optimization for repeated boolean configurationsThe boolean spell configurations are identical across coding and noncoding sections. Consider extracting these common configurations into a reusable YAML anchor to reduce duplication.
Example optimization:
# At the top of the file __definitions__: boolean_spell: &boolean_spell url: v1.2.1/logic/boolean with: true_value: "True" false_value: "False" # In the configuration canonical: spell: *boolean_spell mane_plus_clinical: spell: *boolean_spellAlso applies to: 488-491
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml
(3 hunks)
🔇 Additional comments (3)
workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml (3)
309-312
: LGTM: Boolean spell configuration is now more explicit
The update properly defines true/false values for the boolean spell, improving clarity and consistency.
317-320
: LGTM: Consistent boolean configuration
The boolean spell configuration matches the canonical flag, maintaining consistency across the configuration.
205-206
: Verify the new clinical significance spell functionality
The update to use the specialized clin_sig
spell (v1.2.1) aligns with the PR objectives. This change should provide better visualization of clinical significance data.
Let's verify the spell's existence and compatibility:
✅ Verification successful
The clinical significance spell update is appropriate and consistent with the codebase
The verification shows that:
- The clinical significance functionality is properly implemented in
clinical_significance.js
with color mappings for different significance levels - The spell update to
v1.2.1/med/clin_sig
is consistent with other v1.2.1 spells used in the same file (e.g., for boolean values) - The clinical significance column is properly integrated in the datavzrd configuration and is marked as optional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and compatibility of the new clin_sig spell
# Check if the new spell version exists in the repository
rg -l "v1.2.1/med/clin_sig" .
# Look for any existing usage patterns of clinical significance
rg "'clinical significance':" -A 5 -B 5
Length of output: 77
Script:
#!/bin/bash
# Let's try different search patterns to understand the clinical significance configuration
# Search for clin_sig in any context
rg "clin_sig" -A 3 -B 3
# Search for clinical significance without quotes
rg "clinical.?significance" -A 3 -B 3
# Look for spell configurations in the datavzrd yaml
fd ".*\.datavzrd\.yaml$" --exec grep -A 5 -B 5 "spell:"
# Check if there are any other version references
rg "v1\.2\.1" -A 3 -B 3
Length of output: 2134
Script:
#!/bin/bash
# Let's examine the clinical_significance.js file and check the datavzrd yaml file content
# Check the content of clinical_significance.js
cat workflow/resources/datavzrd/clinical_significance.js
# Examine the full variant-calls-template.datavzrd.yaml
cat workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml
# Look for any documentation about spells
fd "README|CHANGELOG" --exec grep -A 5 -B 5 -i "spell"
Length of output: 17871
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Still, tidying up the code base would be nice. The js-file which got replaced by the clin-sig spell is not required anymore and therefore can be deleted. Also, the input of the datavzrd rule pointing to that js-file needs to be modified.
workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml (1)
309-312
: LGTM: Boolean spell configurations consistently updatedThe boolean spell configurations have been consistently updated across all instances with:
- Version upgrade to v1.2.1
- Explicit false_value setting
Consider extracting the common boolean spell configuration into a YAML anchor to reduce duplication. Example:
# At the top of the file __definitions__: boolean_spell: &boolean_spell url: v1.2.1/logic/boolean with: true_value: "True" false_value: "False" # Then in the configurations canonical: spell: *boolean_spellAlso applies to: 317-320, 480-483, 488-491
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml
(3 hunks)
🔇 Additional comments (2)
workflow/resources/datavzrd/variant-calls-template.datavzrd.yaml (2)
205-206
: LGTM: Clinical significance spell version updated as suggested
The update to v1.2.1 for the clinical significance spell has been implemented as previously suggested.
Line range hint 341-371
: LGTM: Well-defined visualization configurations
The visualization configurations for SpliceAI and AlphaMissense scores are well-structured with:
- Clear domain boundaries for score ranges
- Appropriate color schemes for representing different ranges
- Consistent configuration between coding and noncoding sections
Also applies to: 531-561
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine now! Thanks :)
An updated version of datavzrd is used for the boolean spell, which now visualizes via badges. The clinical significance function is replaced by the clin_sig spell.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
clinical_significance.js
file and associated functions to streamline the codebase.